Skip to content

Conversation

@tkrugg
Copy link
Contributor

@tkrugg tkrugg commented Oct 23, 2018

upgraded [email protected] and [email protected].
pinned yarn version in travis

@algobot
Copy link
Contributor

algobot commented Oct 23, 2018

Deploy preview for algolia-instantsearch ready!

Built with commit 40488bf

https://deploy-preview-3228--algolia-instantsearch.netlify.com

@francoischalifour francoischalifour changed the title core(engine): pin node and yarn version chore(engine): pin Node and Yarn version Oct 23, 2018
@tkrugg tkrugg requested review from a team and samouss October 23, 2018 09:55
@tkrugg
Copy link
Contributor Author

tkrugg commented Oct 23, 2018

The motivation for doing this is avoid conflicts of integrity sha in yarn.lock. This what react-instantsearch seem to be doing.

I'm not sure however how this is going to play out for user already using instantsearch.js. Would it make there update more painful? what if they have a more recent version of node and yarn, doesn't this constraint them to downgrade?

.travis.yml Outdated
before_install:
- git config --global user.email "[email protected]"
- git config --global user.name "algobot"
- curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version 1.10.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Those are not indented correctly (so are the two previous lines for some reason).

package.json Outdated
"vanilla"
],
"engines": {
"node": "8.12.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

just make sure that this doesn't end up in the package.json that gets pushed to npm, worst case, filter it out before publishing

Copy link
Contributor

@francoischalifour francoischalifour Oct 23, 2018

Choose a reason for hiding this comment

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

Let's remove it since it doesn't bring anything more than having it in .nvmrc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure to understand your comment @Haroenv. Why it would have an impact on the published package? The engines attribute will only throw an error when a script is ran on that package specifically otherwise it's only a warning. It will only impact contributors.

@francoischalifour It does bring more value because not everyone is using nvm. For those who don't, the attribute is useful. One question remains on the strictness of the value. We can probably use a range rather than a pinned version. It will still enforce a value without bothering too much the contributors.

@tkrugg tkrugg merged commit c562f4d into develop Oct 23, 2018
@samouss samouss deleted the engine branch October 23, 2018 16:57
@francoischalifour
Copy link
Contributor

francoischalifour commented Oct 26, 2018

Since the PR was rebased it doesn't pin Yarn for us developers (in package.json) but only for Travis and Netlify.

francoischalifour pushed a commit that referenced this pull request Oct 26, 2018
This also upgrades Node to 8.12.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants