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

feat: add support for additional node.js builtin modules #12181

Merged
merged 2 commits into from Dec 14, 2020

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Dec 12, 2020

What kind of change does this PR introduce?

feature, node.js builtin support for "timers/promises"

feature, adds support for dns/promises, stream/promises, timers/promises, wasi

https://nodejs.org/api/timers.html#timers_timers_promises_api

Did you add tests for your changes?

no.

Does this PR introduce a breaking change?

no, additive.

What needs to be documented once your changes are merged?

n/a

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@alexander-akait
Copy link
Member

@dnalborczyk Can you accept CLA, and let's add all promises modules, for example fs module support promises too

@sokra sokra closed this Dec 14, 2020
@sokra sokra reopened this Dec 14, 2020
@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Dec 14, 2020

@alexander-akait I accepted the CLA, although it seemed to be somewhat buggy around the time I created the PR. it seems @sokra fixed it with closing + reopening.

let's add all promises modules, for example fs module support promises too

fs/promises already exists:

"fs/promises",

timers/promises was added in node v15.x, whereas fs/promises was exposed in node.js v14.x.

if you know about any additional missing paths I can add them in this PR as well, otherwise I think they can be added in subsequent PRs if an issue should arise.

@alexander-akait
Copy link
Member

@dnalborczyk yep, but let's fix all of them in the one PR, need to add:

  • dns/promises
  • stream/promises

Also I found Node.js modules can be used node:fs/promises (but let's solve it in separate PR)

@alexander-akait
Copy link
Member

Let's fix lint problems

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Dec 14, 2020

Let's fix lint problems

I also added wasi, which seems to cause a spell checking problem. I'll fix it.

Not sure if there's a pattern used for the webpack project yet?

it seems to work if applied anywhere in the code, e.g.

const a = [
	...
	"wasi", // cSpell:ignore wasi
	...
]

or better on top of the file? or do we have a global words file?

@webpack-bot
Copy link
Contributor

@dnalborczyk Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@alexander-akait Please review the new changes.

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Dec 14, 2020

in the interim I added the spellcheck ignore next to the wasi entry, but I can change it to something else if there has been a pattern already established.

@dnalborczyk dnalborczyk changed the title feat: add support for node.js v15 builtin timers/promises feat: add support for additional node.js builtin modules Dec 14, 2020
@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Dec 14, 2020

Also I found Node.js modules can be used node:fs/promises (but let's solve it in separate PR)

interesting, I didn't know it found its way into node.js already.

https://github.com/tc39/proposal-built-in-modules#namespace
https://developers.google.com/web/updates/2019/03/kv-storage#what_are_built-in_modules

I'll have a look into this (and possible future PR).

@sokra sokra merged commit 0c4d47a into webpack:master Dec 14, 2020
@sokra
Copy link
Member

sokra commented Dec 14, 2020

Thanks

@dnalborczyk dnalborczyk deleted the node-builtins branch December 15, 2020 02:20
@dnalborczyk
Copy link
Contributor Author

@sokra @alexander-akait

node: is an URL scheme and therefore not working for commonjs.
https://nodejs.org/api/esm.html#esm_node_imports

I think support should be added once webpack supports esm modules as output.

I suppose eventually (if the need arises), webpack could also support http: (and possibly other) URL scheme's for node.js as a target.

https://nodejs.org/api/esm.html#esm_urls

@alexander-akait
Copy link
Member

I think support should be added once webpack supports esm modules as output.

I think it is not related to output format

I suppose eventually (if the need arises), webpack could also support http: (and possibly other) URL scheme's for node.js as a target.

webpack suppots http and https https://github.com/webpack/webpack/blob/165fa87a69f7ec11aac1fc1ffe8ab1cdb94a8545/test/configCases/asset-modules/https-url/webpack.config.js

@sokra Should we add this as plugin?

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Dec 15, 2020

I think support should be added once webpack supports esm modules as output.

I think it is not related to output format

I gave it a spin, it only works with ES6 modules.

when run in node.js v15.4.0 (without webpack), with package.json set to type: module:

// works
import { readFile } from 'node:fs'

commonjs:

// does not
const { readFile } = require('node:fs')

when webpack bundles to anything other than esm (which it does not support yet), it won't work.

@alexander-akait
Copy link
Member

@dnalborczyk webpack accepts es module syntax, so developer can use node:fs in own code

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Dec 15, 2020

@dnalborczyk webpack accepts es module syntax, so developer can use node:fs in own code

if I'm not mistaken, currently, webpack transpiles (bundles) esm to commonjs (and other formats, but not esm).

related: #2933

in order to have it working you'd need to bundle esm to esm.

@alexander-akait
Copy link
Member

I mean webpack can resolve node:fs in ES modules with target: 'node'

@dnalborczyk
Copy link
Contributor Author

@alexander-akait

the prefixed commonjs modules are currently being added to node.js as well: nodejs/node#37246

@alexander-akait
Copy link
Member

@dnalborczyk can you open an issue issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants