Skip to content

Add http2 to builtins - #1913

Merged
goto-bus-stop merged 1 commit into
browserify:masterfrom
steve-taylor:master
Jul 5, 2019
Merged

goto-bus-stop merged 1 commit into
browserify:masterfrom
steve-taylor:master

Conversation

@steve-taylor

@steve-taylor steve-taylor commented Jul 5, 2019

Copy link
Copy Markdown

Node.js 10.10.0 added the http2 module. This change adds just enough support for --no-builtins (and therefore --node) to recognise http2 as a Node.js built-in module and not attempt to bundle it.

@goto-bus-stop goto-bus-stop left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@goto-bus-stop
goto-bus-stop merged commit 9e3397b into browserify:master Jul 5, 2019
@steve-taylor

steve-taylor commented Jul 5, 2019

Copy link
Copy Markdown
Author

You're welcome. Thanks for the quick merge and release!

@ljharb

ljharb commented Jul 5, 2019

Copy link
Copy Markdown
Member

It might be worth using resolve’s listing of core modules to default all of them to empty (and override with shims as needed) - then this would have Just Worked.

@goto-bus-stop

Copy link
Copy Markdown
Member

that sounds like a good idea—we've had a few browserify releases lately that were mostly about adding new empty shims.

@ljharb

ljharb commented Jul 5, 2019

Copy link
Copy Markdown
Member

(Pull in resolve/lib/core, enumerate the entries, and any key whose value is true is a core module in the current node version)

@steve-taylor

Copy link
Copy Markdown
Author

The set of core modules isn’t the same for all Node versions. We might not necessarily build and run on the same version of Node.

Maybe we need an extra option to specify the target runtime, e.g. —node-version 10.16.0.

@ljharb

ljharb commented Jul 6, 2019

Copy link
Copy Markdown
Member

@steve-taylor that’s a reasonable addition to resolve if it’s needed; but i do think that building and running on the same version is typical.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants