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

fix: correct minimum node version to one that supports modules #1085

Merged
merged 2 commits into from
Dec 3, 2023
Merged

fix: correct minimum node version to one that supports modules #1085

merged 2 commits into from
Dec 3, 2023

Conversation

mreinstein
Copy link
Contributor

Currently our package.json says minimum node engine version is 8, but we have "type": "module" in our package.json and that requires node v12+ as per https://nodejs.org/dist/latest-v20.x/docs/api/esm.html#modules-ecmascript-modules

Screenshot 2023-11-30 at 9 42 15 AM

@iambumblehead
Copy link
Contributor

The current published project is type "commonjs" and not type "module" and that is seen at the package.json here https://www.npmjs.com/package/snabbdom?activeTab=code

Maybe the published Snabbdom sources have type "module" removed from them, I don't know...

@kuraga
Copy link

kuraga commented Nov 30, 2023

@mreinstein , why was there 12.18? #1080 (comment)

@mreinstein
Copy link
Contributor Author

It depends on how one defines support. technically it will work in node > 12.0, but that requires a command line flag. 12.17 and up work without the command line flag.

@mreinstein
Copy link
Contributor Author

The current published project is type "commonjs" and not type "module"

That's because the last publish to npm was over a year ago, but the commit that introduced "type": "module" was only about a month ago:

0a4fb34

So this PR will ensure that the node version matches the absolute minimum required value. It's questionable if maybe this should actually be 12.17.0 as kuraga pointed out.

@iambumblehead
Copy link
Contributor

I have experience using type module with node v12 and modules do work there

@iambumblehead
Copy link
Contributor

note: if dependents use other files than the main exported file, those other files must be explicitly exported at the package.json.

Should exports for these files be added to the package.json?

snabbdom木$ tree build/helpers/
build/helpers/
├── attachto.d.ts
├── attachto.js
└── attachto.js.map

@mreinstein
Copy link
Contributor Author

Should exports for these files be added to the package.json

Out of scope for this PR, I want to keep this focused on fixing one singular problem.

@iambumblehead

This comment was marked as off-topic.

@mreinstein
Copy link
Contributor Author

mreinstein commented Nov 30, 2023

@jvanbruegge note that the next snabbdom deploy npm publish should bump semver major, since we've changed the module type and the minimum node requirements. Might be nice to do this as a lean 4.0.0 release, rather than cramming a lot of other changes in with it.

@paldepind
Copy link
Member

The minimum Node version is a bit artificial as Snabbdom is a browser library and doesn't work in Node.

That being said if a version of Node can't import Snabbdom then it certainly doesn't support Snabbdom, so bumping the version seems reasonable. I do wonder though if removing the "engine" entry altogether would have any ramifications? It was added in #948.

@mreinstein
Copy link
Contributor Author

mreinstein commented Dec 3, 2023

the minimum Node version is a bit artificial as Snabbdom is a browser library

"type": "module" in package.json is also a purely node feature. With these 2 pieces of node-only metadata we're declaring a config that isn't possible. Converted to English our node metadata says something like "We use es modules. Make sure you are running this module on a version of node that existed way before es modules were supported, or higher"

and doesn't work in Node.

You have to be careful with assumptions. Server side rendering is one case where snabbdom might be invoked outside of a browser. The only parts of snabbdom that actually interface with the browser are neatly encapsulated in src/htmldomapi.ts, and it's really quite trivial to stub out that very limited api on window. and document. globals to be able to invoke snabbdom server-side (without needing heavyweight puppeteer, etc.)

I do wonder though if removing the "engine" entry altogether

Can you explain why there is a hesitancy around bumping the node version up to 12.something ? This seems like a non-issue unless I'm missing something.

@paldepind
Copy link
Member

Those are very good points 👍. I didn't think about server-side rendering – proving that you're absolutely right about not making too many assumptions.

Thanks for the PR.

"type": "module" in package.json is also a purely node feature.

It also serves as info that web bundlers can use. Webpack, for instance, uses that field to determine which module format to use.

@paldepind paldepind merged commit 5fa4e84 into snabbdom:master Dec 3, 2023
2 checks passed
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.

None yet

4 participants