Skip to content

Update yarn.lock and use node 6.0 on Travis#10671

Closed
rsimha wants to merge 3 commits intoampproject:masterfrom
rsimha:2017-07-24-YarnLock
Closed

Update yarn.lock and use node 6.0 on Travis#10671
rsimha wants to merge 3 commits intoampproject:masterfrom
rsimha:2017-07-24-YarnLock

Conversation

@rsimha
Copy link
Copy Markdown
Contributor

@rsimha rsimha commented Jul 27, 2017

@Raphaeljunior is running into issues with missing dependencies on his linux machine, and when I was helping him debug, I noticed that running yarn upgrade --save-dev resulted in a lot of updates to yarn.lock. It probably makes sense to check this in.

/to @erwinmombay for review / comment.

Fixes #10673

@rsimha rsimha self-assigned this Jul 27, 2017
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Jul 27, 2017

@erwinmombay, this is currently failing because we now depend on punycode which is incompatible with node 4.0. Any idea how to fix the yarn dependency so it's not automatically pulled in by yarn upgrade --save-dev?

Edit: Upgraded the node version on Travis from 4.0 to 6.0

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Jul 27, 2017

@jridgewell, was there an issue associated with #9575? Can you remind me of why this was necessary?

I'm seeing a bunch of errors on Travis while updating yarn.lock due to some dependencies being incompatible with node 4.0. Turns out they need a node version >= 6.0. I'm going to switch our node version on Travis to 6.0 unless there's a good reason for sticking with node 4.0. IIRC, the node bug we ran into was in version 8.0.

@rsimha rsimha changed the title Update yarn.lock Update yarn.lock and use node 6.0 on Travis Jul 27, 2017
@rsimha rsimha requested a review from jridgewell July 27, 2017 16:30
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Jul 27, 2017

Latest commit worked. We now need node 6.0 on Travis. This is ready for review.

dist: trusty
node_js:
- "4"
- "6"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We support the latest LTS, because it's used everywhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know what an LTS is, but don't understand what this means for our repository, or what Erwin's thumbs up means :)

Do we want to stick with node 4.0? This means that yarn.lock will be permanently out of date, given that we cannot update it until we move off of node 4.0. Is that acceptable? Won't new committers run into dependency problems while developing because yarn.lock is out of date?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is now blocking #10776 due to fetch-mock being incompatible with node 4. See https://travis-ci.org/ampproject/amphtml/jobs/260683377

Using node 6 for testing on Travis solves these issues.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for linking that @rsimha-amp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you also update package.json engine version

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nevermind just read your latest comment :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@erwinmombay, I've sent you #10781 to detect issues like this in future.

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Aug 3, 2017

/to @choumx for review

@rsimha rsimha requested a review from dreamofabear August 3, 2017 17:34
@rsimha rsimha mentioned this pull request Aug 3, 2017
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Aug 3, 2017

Turns out that after a week of hacking at this, we must run yarn install and not yarn upgrade. Closing this, and sending out a new PR to uptate documentation.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants