Require Node.js 12.20 and move to ESM #472
Conversation
|
@Borewit Any idea why some of the test are failing? |
| fromStream as tokenizerFromStream, | ||
| fromBuffer as tokenizerFromBuffer, | ||
| EndOfStreamError, | ||
| } from 'strtok3/lib/core.js'; |
There was a problem hiding this comment.
@Borewit This kind of deep import is discouraged with ESM. Instead, you should use the exports field in package.json to properly export it. Can you do that? It will require a major release as implicit sub-exports like this will no longer be available then.
https://nodejs.org/api/packages.html#packages_subpath_exports
There was a problem hiding this comment.
I will try to find a solution with ESM export: Borewit/strtok3#530
|
I'm merging this new to avoid a lot of conflicts. I'll look into the test failures soon. |
| @@ -10,8 +10,10 @@ | |||
| "email": "sindresorhus@gmail.com", | |||
| "url": "https://sindresorhus.com" | |||
| }, | |||
| "type": "module", | |||
| "exports": "./index.js", | |||
There was a problem hiding this comment.
@sindresorhus is importing /core no longer supported? (or maybe it wasn't ever supported 😅)
My reason for importing /core is that my package should be compatible with both Node and browser, and I don't accidentally want to use something only available on one of the platforms...
There was a problem hiding this comment.
It wasn't ever supported. I'm actually removing the /browser sub-export in favor of just the default export with automatic selection for Node.js and browser: 287e361
I personally don't want to expose core for simplicity and it's also not something I want to expose. Maybe some day we'll be able to unify more of the methods and exposing core makes it more difficult to make changes. For example, Node.js will soon get Blob support, so fileTypeFromBlob will eventually work in Node.js too.
@Borewit Thoughts?
There was a problem hiding this comment.
The default module was designed for Node.js, and we added a sub-import for the browser in '/browser'.
This was introduced at the point we start to depend on Node.js specific file access, causing interoperability issues for the browsers (respectively the module bundlers like Webpack).
As part of this separation, /core was used to hold shared functionality between Node.js & browser.
To leverage on /core to build further on shared code is an understandable approach. I cannot think of a reasonable alternative. We have done exactly the same using strtok3/core.
I would like to keep supporting all of the 3 use cases: node.js, browser, and universal code. I am in learning mode with this new ESM features, and in your previous response @sindresorhus I see that there is some automatic selection. That seems like a logical thing to me, but I was completely unaware of any semantic interpretation of the sub modules names.
I have to say that so far I am not very impressed by the entire ES Modules. Poorly documented, the claimed backward CommonJS compatibility is arguable. The one module good for all, seems so far turning into a module for the happy few. I know sometimes it's doing 1 step back to be able to do 2 steps forward, but I am super impressed.
Long story short, I am in favor of supporting @LinusU, by exposing /core sub-module unless there is better workable alternative to get the job done.
There was a problem hiding this comment.
I'm okay with either outcome 👍
Also, if there is anything I can do to help get this release out of the door I'm happy to pitch in!
There was a problem hiding this comment.
but I was completely unaware of any semantic interpretation of the sub modules names.
I recommend reading https://nodejs.org/api/packages.html. It's all documented there.
Poorly documented
It is well documented: https://nodejs.org/api/esm.html
by exposing /core sub-module unless there is better workable alternative to get the job done.
The alternative is to just import the main export. The only difference is that you have to make sure yourself not to import, for example, {fileTypeFromFile} if you want the code to work everywhere.
Also, if there is anything I can do to help get this release out of the door I'm happy to pitch in!
The current blocker is figuring out why the tests are failing. I haven't had a chance to do that yet, and I'm on summer vacation, so I don't have a lot of available time.
There was a problem hiding this comment.
That kind of means file-type becomes a browser module by default.
That's technically correct, but it's just because node is a defined environment, but browser is not, so we use default for the browser, since we only support these two types of platforms.
Are we then responsible for poly-filling everything which is node specific e.g. node:stream & node:buffer ?
If we use anything Node.js specific In browser.js, then yes, but we already had file-type/browser which worked fine, so I don't see how this change changes anything, other than not having to use the sub-export.
Related to this, dependency readable-web-to-node-stream should only be used in default (browser) environment.
Node.js recently got web streams support, so eventually, we'll only have a single code-path for the stream stuff.
There was a problem hiding this comment.
but we already had file-type/browser which worked fine
It worked fine in combination with a module bundler (e.g. webpack).
The difference being that the module bundler would polyfill the typical Node.js dependencies.
I wondered if the "sort out the Node.js polyfills yourself in your module bundler" still flies.
There was a problem hiding this comment.
The difference being that the module bundler would polyfill the typical Node.js dependencies.
Webpack 5 no longer polyfills Node.js APIs.
There was a problem hiding this comment.
So in principle we should take care of all dependencies in non-node / default / browser case, right?
Would be good to CI test the default path.
There was a problem hiding this comment.
I personally don't care about the browser case. It's a "best effort" thing.
TODO:
fromBuffer=>fileTypeFromBufferto make them more readable.