Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Build: remove npm-shrinkwrap.json and try yarn #8660

Closed
wants to merge 3 commits into from
Closed

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Oct 11, 2016

An in progress branch to see if we can use yarn and ditch npm-shrinkwrap.json

@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=try/yarn

RUN npm install --production || npm install --production
COPY ./yarn.lock /calypso/yarn.lock

RUN curl -o- -L https://yarnpkg.com/install.sh | bash && export PATH="$HOME/.yarn/bin:$PATH" && yarn install
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to wait until @dmsnell's docker changes are in before debugging this too far

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just install it using npm install yarn --global . Somehow I find the curl | bash installation method too magic :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about npm install yarn=0.15.1 -g --prefix ./node_utils and then run with ./node_utils/bin/yarn?

This way we can control the version we are using and not clutter global.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoa! where's that global flag coming from - stop messing up my system! 😄

@gwwar in relation to the Dockerfile we may have more issues with the deployment process than code conflicts because it specifically sets a local npm mirror.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly meant that we should wait since our base image is changing from Debian to node-slim with @dmsnell upcoming changes. If you look at https://yarnpkg.com/en/docs/install#linux they also provide the package via apt-get, but I was running into trouble configuring the apt-get repository to be able to download yarn.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self, this should also be run with yarn install --production to skip over devDependencies. https://yarnpkg.com/en/docs/cli/install

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yarn install --production to skip over devDependencies

probably not @gwwar because the Docker container builds the bundles from source. we may not need linting but we certainly need the loaders and babel etc…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use something in both prod/dev, that makes it a plain dependency 😄 If you look at our package.json, babel should be under dependencies vs devDependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm waiting until #8603 lands before making additional changes in the docker file. Though feel free to experiment 😄

@gwwar
Copy link
Contributor Author

gwwar commented Oct 11, 2016

Odd, one failing test with yarn vs npm.

screen shot 2016-10-11 at 3 48 22 pm

@gwwar
Copy link
Contributor Author

gwwar commented Oct 12, 2016

Hmm got the build green, but running make test locally for me causes node to run out of memory.
screen shot 2016-10-12 at 2 12 18 pm

@@ -80,7 +81,10 @@ welcome:
@printf "\033[36m |___/|_| \n"
@printf "\033[m\n"

install: node_modules
yarn_available:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like an odd name for a make target. it's almost as if it should return some boolean indication, but it's really just forcing a dependency. maybe something like demand_yarn or yarn_check would fit better

@dmsnell
Copy link
Member

dmsnell commented Oct 12, 2016

@gwwar why do we need yarn installed globally? is it possible to use a local copy and npm install yarn before building? personally I've dealt with plenty of headaches in the past two months because of global version clashes and that has been so frustrating to resolve

expect( payment.type ).to.equal( 'credit_card' );
expect( payment.countryCode ).to.equal( 'US' );
expect( payment.countryName ).to.equal( 'United States' );
expect( payment.name ).to.equal( 'My VISA' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a win outside of this PR too, no? could we move these changes into a quick-fix PR and just push it out there? it doesn't really seem to fit well with the yarn stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in #8698

@@ -1,6 +1,11 @@
machine:
node:
version: 6.7.0
dependencies:
pre:
- curl -o- -L https://yarnpkg.com/install.sh | bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

versioning much? I think we could pull from https://github.com/yarnpkg/yarn/releases if we wanted to pin it and make updates their own PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm interesting thought for fixing yarn versions. Should we maybe try to bump to latest on each node bump?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. Having it versioned makes for nice explicit changes in PRs

@gwwar
Copy link
Contributor Author

gwwar commented Oct 12, 2016

@gwwar why do we need yarn installed globally?

I can give that a shot. Was doing some quick hacks yesterday ;D

Edit: note that we'll see this annoying prompt npm WARN prefer global [email protected] should be installed with -g since "preferGlobal": true, is set in their package.json

@dmsnell
Copy link
Member

dmsnell commented Oct 13, 2016

Edit: note that we'll see this annoying prompt npm WARN prefer global [email protected] should be installed with -g since "preferGlobal": true, is set in their package.json

yuck. it's probably okay if we need to. I see the causal dependence on npm that could make this tricky and justify a global. globals can be just so nasty

@umurkontaci
Copy link
Contributor

I said this above but got no response, what about:

npm install yarn=0.15.1 -g --prefix ./node_utils and then run with ./node_utils/bin/yarn? AFAIK, --global puts an executable inside the bin folder for node. If we use --prefix along with --global, it will install everything inside the folder we specify, without leaking.

We can then .gitignore that folder and control the yarn version from Makefile.

@dmsnell
Copy link
Member

dmsnell commented Oct 14, 2016

sorry if I missed your comment @umurkontaci but that sounds like a great idea

now, let's pause and consider our other globals. how many (if any) do we depend on? could this same solution make all of them non-global?

@umurkontaci
Copy link
Contributor

umurkontaci commented Oct 14, 2016

sorry if I missed your comment @umurkontaci but that sounds like a great idea

No worries! 👍 Also double checked that it does not link to any folder in $PATH when installed with a prefix.

how many (if any) do we depend on? could this same solution make all of them non-global?

Apart from shonkwrap, I don't think we have any.

$ npm ls -g --depth=0 2>/dev/null
/usr/local/lib
`-- [email protected]

After make && make run on fresh docker image node:6.7.0

@gwwar
Copy link
Contributor Author

gwwar commented Oct 14, 2016

Just FYI It's poorly documented at the moment, but @blowery mentioned in the hangout that yarn stores a cache at ~/.yarn-cache which is why it suggests the global flag.

@umurkontaci I think that approach looks good, but we'll want to check where it's placing the cache directory

@umurkontaci
Copy link
Contributor

Good catch. Per https://github.com/yarnpkg/yarn/blob/master/bin/yarn.js#L11 it will use the home directory of the user regardless.

Home directory is $HOME in Unix and $USERPROFILE in Win32. * If we really want, we can set $HOME to a different value while calling yarn. I don't see this being a huge issue as far as yarn is concerned, but it makes me very uncomfortable to change $HOME.

We can probably fix this in the upstream by adding an option to set the cache file name from an env value or an optional command.

@gwwar
Copy link
Contributor Author

gwwar commented Oct 14, 2016

We can probably fix this in the upstream by adding an option to set the cache file name from an env value or an optional command.

Thanks for 👀 @umurkontaci. That seems like a good step to take.

We can probably steal a few ideas from yarnpkg/yarn#749 as well

@dmsnell
Copy link
Member

dmsnell commented Nov 6, 2016

@gwwar any reason this is on hold? are we waiting for more stabilization in yarn? I think we can 🚢 it with yarn installed globally. it's still a net positive.

@gwwar
Copy link
Contributor Author

gwwar commented Nov 7, 2016

I'm waiting for yarn to stabilize a bit more. There are open issues like yarnpkg/yarn#761 that I'd not like to debug in prod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants