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

package.json "engine" is too restrictive #359

Closed
chandlerprall opened this issue Aug 28, 2019 · 11 comments · Fixed by #363
Closed

package.json "engine" is too restrictive #359

chandlerprall opened this issue Aug 28, 2019 · 11 comments · Fixed by #363
Labels
bug Something isn't working released Issue released publicly

Comments

@chandlerprall
Copy link

The node version in the package.json's engines field is too restrictive to include elastic-charts in other projects. For example, EUI's CI breaks when trying to add elastic charts as we are not on that exact 10.15.2 node version. Even if we were to update and track this version requirement, we'd be forcing all consumers of EUI to use that specific node version, even in non-Kibana use cases.

@chandlerprall chandlerprall added the bug Something isn't working label Aug 28, 2019
@markov00
Copy link
Member

Hey @chandlerprall
I think we need to change the .nvmrc config instead.
Quoting https://docs.npmjs.com/files/package.json#engines

Unless the user has set the engine-strict config flag, this field is advisory only and will only produce warnings when your package is installed as a dependency.

So I think it's our .nvmrc that create issues with the CI

@markov00
Copy link
Member

...and rethinking about my last comment, it's also more strange because the .nvmrc is not inside the distributed package, but only on the repo

@markov00
Copy link
Member

ok so actually that note is only for npm and doesn't apply to yarn.
On yarn the that thing is only applied when passing --ignore-engines
Let me check to what version we can downgrade to satisfy our packages and I will update the library

@chandlerprall
Copy link
Author

Thanks for looking into the npm-yarn differences! I'd also note:

  • if we were using npm, I would prefer users of EUI not getting even a warning message
  • with yarn, I don't think it would be good developer experience to recommend (or require!) --ignore-engines be used by consumers

@markov00
Copy link
Member

So I've investigated a bit more the issue, here my findings:

So the problem now is: it doesn't matter if we remove the engines configuration on the release package.json, we also have to get remove of the commitizen dependency at least. This, btw, will not completely create a deterministic build because there can maybe be shared dependencies between the existing packages.

Maybe the solution is completely remove the commitizen CLI from the package. @nickofthyme what do you think?

@jbudz
Copy link
Member

jbudz commented Aug 29, 2019

Hmm - related, I'm trying to upgrade kibana's node version now and it's being blocked on this. It looks like CI is installing kibana w/ a circular depdency on node version mismathes now :). LMK if I can help

@jbudz
Copy link
Member

jbudz commented Aug 29, 2019

Maybe engines in this repo could indicate what versions of node this package supports, so it would be >=10 or something?

nickofthyme added a commit to nickofthyme/elastic-charts that referenced this issue Aug 29, 2019
Set node engine to >=10

closes elastic#359
markov00 pushed a commit that referenced this issue Aug 30, 2019
Set node engine to >=10

closes #359
markov00 pushed a commit that referenced this issue Aug 30, 2019
## [11.1.2](v11.1.1...v11.1.2) (2019-08-30)

### Bug Fixes

* **engines:** update node engine ([#363](#363)) ([7fcd98c](7fcd98c)), closes [#359](#359)
@markov00
Copy link
Member

🎉 This issue has been resolved in version 11.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Aug 30, 2019
@markov00
Copy link
Member

Hey @chandlerprall we changed the engines to a less restrictive version >=10.
We cannot easily go down to version 8 because was required by commitizen. We need to keep the commitizen adapter in the package because it's required, but this will pull in the commitizen lib with the node >=10 requirement

@chandlerprall
Copy link
Author

@markov00 was there no luck/success with publishing the package with no engines field?

@nickofthyme
Copy link
Collaborator

nickofthyme commented Sep 3, 2019

Just to update per slack conversation

  • Keeping engines at node@10 for the meantime
  • Long term solution switch to yarn workspace and remove engines altogether

To discuss with @markov00 when he returns

AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this issue Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released Issue released publicly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants