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

[Discussion] Clarify which node versions are supported and add `engines` filed into `package.json` #878

Open
JounQin opened this issue Dec 20, 2019 · 19 comments

Comments

@ChristianMurphy

This comment has been minimized.

Copy link
Member

@ChristianMurphy ChristianMurphy commented Dec 20, 2019

Related, recently unified had a similar discussion about adding engines unifiedjs/unified#70
Unified decided not to, due to the scale of packages that would need to be updated, and some issues with state versions not lining up with real support.
MDX, having a different project structure and user base, may come to a different decision.

@JounQin

This comment has been minimized.

Copy link
Member Author

@JounQin JounQin commented Dec 21, 2019

@ChristianMurphy Then I think node: * is also fine to clarify these packages are for node only?

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Dec 21, 2019

What is the difference between node: * and no engines field at all?
Many packages can be used in other engines than Node?

@JounQin

This comment has been minimized.

Copy link
Member Author

@JounQin JounQin commented Dec 21, 2019

@wooorm That clarifies that these packages are for node only. @pkgr/rollup will read it to determine whether to treat dependencies as external just like peerDependendcies for non node packages.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Dec 23, 2019

Why would you want to do that? 🤔 Some packages here work outside of Node.

@JounQin

This comment has been minimized.

Copy link
Member Author

@JounQin JounQin commented Dec 23, 2019

@wooorm So we should clarify which packages work outside of node and which work only on node. I didn't mean to add it at root package.json.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Dec 28, 2019

Can you elaborate? In which cases would that help you?
Most code here can be included in a rollup bundle. It’s not often smart, but there are some cases where it makes sense.
What is the benefit in prohibiting it?

@JounQin

This comment has been minimized.

Copy link
Member Author

@JounQin JounQin commented Dec 28, 2019

Most code here can be included in a rollup bundle.

@wooorm Yep, that's the point. @pkgr/rollup is a rollup wrapper, it will try best to determine which packages should be considered as externals, dependencies will be true on targeting node pkg but will not on targeting browser. Then engines field declares whether the pkg a node pkg instead of browser.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Dec 29, 2019

Most packages here can be used in the browser, they probably shouldn’t, but it should be possible to bundle them. Why would we block that?

The way I understand it, the browser field can be used to create browser-specific versions.

@JounQin

This comment has been minimized.

Copy link
Member Author

@JounQin JounQin commented Dec 30, 2019

@wooorm

Most packages here can be used in the browser

Then we can do not add engines for them, my point is that we can add it for those packages which are really node only.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Dec 30, 2019

Which projects are you proposing to change? create-mdx, parcel-plugin-mdx, webpack loaders?

@JounQin

This comment has been minimized.

Copy link
Member Author

@JounQin JounQin commented Dec 30, 2019

@wooorm The idea came from @mdx-js/runtime, but yeah we can change more packages like you mentioned.

@ChristianMurphy

This comment has been minimized.

Copy link
Member

@ChristianMurphy ChristianMurphy commented Dec 30, 2019

@mdx-js/runtime is one of the packages that can be used in browser.

@JounQin

This comment has been minimized.

Copy link
Member Author

@JounQin JounQin commented Dec 30, 2019

@ChristianMurphy It requires buble-jsx-only, is that really could be used in browser? Or whether its dependencies are all umd available?

@JounQin

This comment has been minimized.

Copy link
Member Author

@JounQin JounQin commented Dec 30, 2019

@ChristianMurphy OK, I saw it at https://unpkg.com/buble-jsx-only@0.19.8/dist/buble-browser-deps.umd.js , and then I'll fix the bundle issue first.

But this issue should be discussed regardless of it, right?

@ChristianMurphy

This comment has been minimized.

Copy link
Member

@ChristianMurphy ChristianMurphy commented Dec 30, 2019

right

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Jan 9, 2020

It feels to me like user-agent sniffing, whereas you should check features instead!

I can’t find it anymore, but there recently was a thread on twitter about someone upgrading to a new version of Node/npm (or an alpha version maybe?), and packages not installing because their engines field didn’t match the new release.
I’d like to prevent stuff like that.

@timneutkens

This comment has been minimized.

Copy link
Member

@timneutkens timneutkens commented Jan 9, 2020

Just to let you know, we use a minimum version on Next.js: https://github.com/zeit/next.js/blob/canary/packages/next/package.json#L195-L197

Newer versions of node are unlikely to break.

@wooorm

This comment has been minimized.

Copy link
Member

@wooorm wooorm commented Jan 9, 2020

How does that work if someone were on 8.11.0-alpha.... I believe that thread I mentioned was about something like that (with this tool I checked that npm’s beta releases aren’t included in such a semver-min range?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.