feature: Installing dependencies with yarn #31

rkostrzewski · 2017-05-21T17:07:46Z

Changes npm install to yarn add when creating app & yarn is available as global command (similar to how create-react-app works). Faster install, progressive enhancement, yada, yada, yada.

Your next Preact PWA starts in less than 30 seconds. 😉

Cross-platform compatibility verified on MacOS & Windows 10.

# Conflicts: # src/commands/create.js

lukeed

Nice!

Perhaps we could refactor away the npm() helper entirely & run everything through spawn()?

rkostrzewski · 2017-05-22T06:56:52Z

Good call.

Also npm-install-loader possibly could use yarn not end up with missing dependencies in yarn.lock file.

rkostrzewski · 2017-05-22T17:17:15Z

src/lib/run-webpack.js

@@ -66,13 +66,12 @@ export function writeJsonStats(stats) {
 	jsonStats.modules.forEach(normalizeModule)
 	jsonStats.chunks.forEach(c => c.modules.forEach(normalizeModule))

-	return fs.writeFile(outputPath, JSON.stringify(jsonStats))


Sneaky refactor 👣

well-earned, I'd say ;)

rkostrzewski · 2017-05-22T17:38:48Z

@lukeed removed npm. ~~but this should still be on hold as postcss-modules-resolve-imports has just upgraded to postcss@6 and yarn installs it as peer dependency and #22 happens all over again~~ fixed.

I've looked into npm-install-loader and I have no idea what does it do.
Firstly, it does not install dependencies (aka other loaders) when other are not present and webpack returns error - I guess that's why #21 was made.
Furthermore, returning early from loader with error doesn't break the build & isn't shown earlier. I've tried following:

let callback = this.async();
callback("Hey I'm an error in a loader!")

What's the point of the loader?

rkostrzewski · 2017-05-22T19:42:29Z

package.json

@@ -94,7 +94,7 @@
    "babel-register": "^6.24.1",
    "copy-webpack-plugin": "^4.0.1",
    "cross-spawn-promise": "^0.10.1",
-    "css-modules-require-hook": "^4.1.0-beta",
+    "css-modules-require-hook": "^4.0.6",


This is the culprit of #22 and issues with yarn. I don't know why beta version was used - last stable version works fine for prerendering (at least for the examples). Don't know if that's ok.

Ooh cool. I'll check this out!

heads up - i merged @lukeed's PR without seeing this first.

thangngoc89 · 2017-05-23T02:31:07Z

I finished review this PR, read several articles on hacker news and it's still installing devDeps. So 👍 👍

developit · 2017-05-25T02:06:52Z

src/commands/create.js

@@ -83,7 +84,7 @@ export default asyncCommand({

 		spinner.text = 'Initializing project';

-		await npm(target, ['init', '-y']);
+		await spawn('npm', ['init', '-y'], { cwd: target, stdio: 'ignore' })


lol this is what it used to be 😆

does Yarn not include an init functionality like npm's?

@developit yes it does

future thing to do might be to move the which check up and use yarn here too. For now anyone running yarn would have npm installed anyway.

developit · 2017-05-25T02:09:49Z

src/lib/run-webpack.js

@@ -66,13 +66,12 @@ export function writeJsonStats(stats) {
 	jsonStats.modules.forEach(normalizeModule)
 	jsonStats.chunks.forEach(c => c.modules.forEach(normalizeModule))

-	return fs.writeFile(outputPath, JSON.stringify(jsonStats))


well-earned, I'd say ;)

developit · 2017-05-25T02:10:56Z

src/commands/create.js

+		if(isDev) {
+			args.push('-D')
+		}
+


We can slim this down by moving the .filter(Boolean) into install, but I'll just slip that in post-merge :)

developit · 2017-05-25T02:13:44Z

@rkostrzewski regarding npm-install-loader - it's less automatic than you're thinking. All it does is install missing dependencies from a manually specified list before continuing to the next loader. If the deps are installed it's a no-op, otherwise it will shell out to install them and then continue.

The use case is: you created a new project and added a .less file. Running compile hits the file and just installs less-loader and friends for you, rather than bailing out. It lets us support typescript/less/sass/etc and configure them internally, but avoids installing them for everyone who may not need or want them in a project.

developit · 2017-05-25T02:17:09Z

@rkostrzewski should we move install() into its own module and import that for use in npm-install-loader? I kept that loader internal for changes like these - we could rename it to dependency-install-loader and have it use npm/yarn same as this PR does.

rkostrzewski · 2017-05-26T08:24:10Z

@developit I don't think it works at all. The less example might work because less-loader is included in preact-cli deps.
Running:

preact create preact-app && cd preact-app
mv ./style/index.css ./style/index.scss
npm run build

Yields:


> [email protected] build /Users/rkostrzewski/GitHub/preact-app
> preact build

./index.js
Module not found: Error: Can't resolve 'sass-loader' in '/Users/rkostrzewski/GitHub/preact-app'
resolve 'sass-loader' in '/Users/rkostrzewski/GitHub/preact-app'
  Parsed request is a module
  using description file: /Users/rkostrzewski/GitHub/preact-app/package.json (relative path: .)
  after using description file: /Users/rkostrzewski/GitHub/preact-app/package.json (relative path: .)
    resolve as module
      looking for modules in /Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules
        using description file: /Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/package.json (relative path: ./node_modules)
        after using description file: /Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/package.json (relative path: ./node_modules)
          using description file: /Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/package.json (relative path: ./node_modules/sass-loader)
            as directory
              /Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules/sass-loader doesn't exist
            no extension
              /Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules/sass-loader doesn't exist
            .js
              /Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules/sass-loader.js doesn't exist
            .json
              /Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules/sass-loader.json doesn't exist
      looking for modules in /Users/rkostrzewski/GitHub/preact-app/node_modules
        using description file: /Users/rkostrzewski/GitHub/preact-app/package.json (relative path: ./node_modules)
        after using description file: /Users/rkostrzewski/GitHub/preact-app/package.json (relative path: ./node_modules)
          using description file: /Users/rkostrzewski/GitHub/preact-app/package.json (relative path: ./node_modules/sass-loader)
            as directory
              /Users/rkostrzewski/GitHub/preact-app/node_modules/sass-loader doesn't exist
            no extension
              /Users/rkostrzewski/GitHub/preact-app/node_modules/sass-loader doesn't exist
            .js
              /Users/rkostrzewski/GitHub/preact-app/node_modules/sass-loader.js doesn't exist
            .json
              /Users/rkostrzewski/GitHub/preact-app/node_modules/sass-loader.json doesn't exist
[/Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules/sass-loader]
[/Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules/sass-loader]
[/Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules/sass-loader.js]
[/Users/rkostrzewski/GitHub/preact-app/node_modules/preact-cli/node_modules/sass-loader.json]
[/Users/rkostrzewski/GitHub/preact-app/node_modules/sass-loader]
[/Users/rkostrzewski/GitHub/preact-app/node_modules/sass-loader]
[/Users/rkostrzewski/GitHub/preact-app/node_modules/sass-loader.js]
[/Users/rkostrzewski/GitHub/preact-app/node_modules/sass-loader.json]
 @ ./index.js 2:0-17
 @ ./~/preact-cli/lib/lib/entry.js
 @ multi ./~/preact-cli/lib/lib/entry

Even tough sass-loader & node-sass are included in npm-install-loader config. That's why I was asking.

I'll try to fix it along while adding yarn to it.

rkostrzewski · 2017-05-26T17:23:37Z

@developit Yeah, I think it won't work and if I'm not wrong there's no way to fix this.

Long story short, webpack verifies if all loaders are present before attempting to compile files. This happens in NormalModuleFactory and occurs before compiler starts compiling & any loaders are run (source + empirical tests). Seemed like a cool feature tough 😢

developit · 2017-05-26T17:26:36Z

Darn. I had figured pitching phase would circumvent that. One other option would be to use a proxy loader - that internally does the installation and then calls out to less-loader.

…arn support

…tall # Conflicts: # src/commands/create.js

rkostrzewski · 2017-05-27T10:58:12Z

Yeah, proxying the loader would work for normal loaders. However it would be hard to make it work with pitch loaders and the solution looks very hacky.

…tall # Conflicts: # src/lib/run-webpack.js

hassanbazzi · 2017-06-02T00:23:11Z

@rkostrzewski I think you can just fix that in webpack by passing a resolveLoader object. I had the same issue happen to me before

rkostrzewski · 2017-06-07T21:19:55Z

@hassanbazzi I don't quite get how to use resolveLoader in this scenario. If I alias loader to another module which exists won't that be used by webpack later on?

Stale review & removed npm() helper

lukeed · 2017-06-07T22:03:25Z

rkostrzewski dismissed lukeed’s review I guess now we're even 😆

…tall # Conflicts: # package.json

…tall # Conflicts: # src/commands/create.js

reznord · 2017-06-28T09:43:09Z

@rkostrzewski Travis tests are failing because of an eslint error. Can I go ahead and fix that error?
It is because const npm = ... here is not being used anywhere !!

But there is this error - https://travis-ci.org/developit/preact-cli/jobs/247829570#L1925-L1926 which I didn't understand.

What is this about?

rkostrzewski · 2017-06-28T15:34:18Z

@reznord sure go ahead. As for the other error I think there's some problem with async commands. I think about spiking the tesrs with console.log to determine where the error is (seems like output folder doesn't exist sometimes on first test)

rkostrzewski · 2017-06-28T15:36:37Z

Also @lukeed has proposed installing with tarn using a switch as new npm has same capabilities as yarn with which I totally agree.

thangngoc89 · 2017-06-28T15:39:14Z

as new npm has same capabilities as yarn

@rkostrzewski I'm not agree with this. Yarn offers much more besides speed and lock file. IMHO, if someone installed yarn it mean that they want to use it.

How about an interactive question to ask which dependency manager to use?

reznord · 2017-06-28T18:04:05Z

@rkostrzewski you have to make the changes, since it is a branch in your local repo :P

reznord · 2017-06-28T18:05:05Z

Also @lukeed has proposed installing with tarn using a switch as new npm has same capabilities as yarn with which I totally agree.

Well, consider this case where you have enabled npm install for running tests. That would just take an eternity to install the node_modules. Considering the same situation with yarn, it caches all the packages and it hardly takes a minute to install the packages.

thangngoc89 · 2017-06-28T18:12:20Z

Well, consider this case where you have enabled npm install for running tests. That would just take an eternity to install the node_modules. Considering the same situation with yarn, it caches all the packages and it hardly takes a minute to install the packages.

@reznord try npm@5 ;)

reznord · 2017-06-28T18:15:06Z

@thangngoc89 I did try, I have to say that it is faster than previous versions but still not close to yarn.

thangngoc89 · 2017-06-28T18:20:52Z

@reznord It's faster than yarn in my exeperience. Maybe your npm@5 global cache is empty? But anyway, see my comment above for my opinion

lukeed · 2017-06-28T18:26:07Z

Chiming in: Yes, I think yarn usage should not be used by default if it is found. Suppose @thangngoc89 had once been a Yarn user, switched to npm@5 but forgot to uninstall Yarn. With this setup, we would find his (old,outdated) Yarn binary and use it.

We should use a --yarn flag because (1) it's technically not an official package manager, (2) it may not always be wanted, (3) presence of a flag is faster & easier to check, plus (4) when the flag is present, we don't have to make any assumptions about whether or not to use it.

Just my 2¢ 😉

thangngoc89 · 2017-06-28T18:33:34Z

@lukeed That seems ok to me

reznord · 2017-06-28T18:35:26Z

Agreed on this !!

developit · 2017-07-01T17:52:01Z

Oooh very good point @lukeed, I'm for the --yarn flag if we want to support installing via it.

rkostrzewski · 2017-07-01T20:42:17Z

@reznord what should I do with this in light of #155? Should I close this and we'll continue with --yarn flag there?

lukeed · 2017-07-01T20:49:22Z

@rkostrzewski Unless I'm hugely mistaken, it doesn't look like that PR is going with a --yarn flag either.

rkostrzewski · 2017-07-01T20:50:18Z

@lukeed yeah I've meant continuing as in developing --yarn flag there

reznord · 2017-07-13T15:41:54Z

A fresh new PR available for yarn support #225 Closing this.

rkostrzewski added 3 commits May 20, 2017 11:25

feat(Yarn): Adds support for installing dependencies with yarn

c4931dd

refactor: generic isCommandAvailable function

7c8ddd3

Merge branch 'developit/master' into feature/yarn-install

130dac3

# Conflicts: # src/commands/create.js

lukeed previously requested changes May 21, 2017

View reviewed changes

refactor: Remove npm helper from create command & use async await

42da57d

rkostrzewski commented May 22, 2017

View reviewed changes

fix: Fix PostCSS 6 issues in dependency chain

e0029ee

rkostrzewski commented May 22, 2017

View reviewed changes

lukeed mentioned this pull request May 22, 2017

Fix test:* scripts #22

Merged

developit reviewed May 25, 2017

View reviewed changes

developit previously approved these changes May 25, 2017

View reviewed changes

rkostrzewski added 2 commits May 27, 2017 12:47

feature(Yarn): npm-install-loader -> dependency-install-loader with Y…

c6a19d1

…arn support

Merge remote-tracking branch 'developit/master' into feature/yarn-ins…

0e0f569

…tall # Conflicts: # src/commands/create.js

Merge remote-tracking branch 'developit/master' into feature/yarn-ins…

d8b57b6

…tall # Conflicts: # src/lib/run-webpack.js

rkostrzewski dismissed developit’s stale review via d8b57b6 June 1, 2017 13:21

Merge remote-tracking branch 'developit/master' into feature/yarn-ins…

2c12da7

…tall # Conflicts: # package.json

rkostrzewski mentioned this pull request Jun 23, 2017

Fix multiple copies of preact in production build (fix #126) #133

Merged

rkostrzewski and others added 4 commits June 23, 2017 22:21

Merge remote-tracking branch 'developit/master' into feature/yarn-ins…

3541f93

…tall # Conflicts: # src/commands/create.js

chore: Remove redundant file

1fc3a4c

fix: Dependency install loader for sass

05c4522

Merge branch 'master' into feature/yarn-install

a6bcbec

reznord mentioned this pull request Jun 29, 2017

feature: Installing dependencies with yarn (continuation) #155

Closed

reznord mentioned this pull request Jul 4, 2017

Update to stage-1 #184

Merged

reznord mentioned this pull request Jul 13, 2017

feature: yarn install (through a flag) #225

Merged

reznord closed this Jul 13, 2017

feature: Installing dependencies with yarn #31

feature: Installing dependencies with yarn #31

Conversation

rkostrzewski commented May 21, 2017

lukeed left a comment

Choose a reason for hiding this comment

rkostrzewski commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkostrzewski commented May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thangngoc89 commented May 23, 2017 • edited Loading

developit May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit commented May 25, 2017 • edited Loading

developit commented May 25, 2017

rkostrzewski commented May 26, 2017

rkostrzewski commented May 26, 2017

developit commented May 26, 2017 • edited Loading

rkostrzewski commented May 27, 2017

hassanbazzi commented Jun 2, 2017

rkostrzewski commented Jun 7, 2017

lukeed commented Jun 7, 2017

reznord commented Jun 28, 2017

rkostrzewski commented Jun 28, 2017

rkostrzewski commented Jun 28, 2017

thangngoc89 commented Jun 28, 2017 • edited Loading

reznord commented Jun 28, 2017

reznord commented Jun 28, 2017

thangngoc89 commented Jun 28, 2017

reznord commented Jun 28, 2017

thangngoc89 commented Jun 28, 2017

lukeed commented Jun 28, 2017

thangngoc89 commented Jun 28, 2017

reznord commented Jun 28, 2017

developit commented Jul 1, 2017

rkostrzewski commented Jul 1, 2017 • edited Loading

lukeed commented Jul 1, 2017

rkostrzewski commented Jul 1, 2017

reznord commented Jul 13, 2017

rkostrzewski commented May 22, 2017 •

edited

Loading

thangngoc89 commented May 23, 2017 •

edited

Loading

developit May 25, 2017 •

edited

Loading

developit commented May 25, 2017 •

edited

Loading

developit commented May 26, 2017 •

edited

Loading

thangngoc89 commented Jun 28, 2017 •

edited

Loading

rkostrzewski commented Jul 1, 2017 •

edited

Loading