Skip to content

Remove requirement for bundler custom config#489

Merged
mcollina merged 3 commits into
nodejs:mainfrom
bergos:zero
Sep 20, 2022
Merged

Remove requirement for bundler custom config#489
mcollina merged 3 commits into
nodejs:mainfrom
bergos:zero

Conversation

@bergos

@bergos bergos commented Sep 14, 2022

Copy link
Copy Markdown
Contributor

The current code requires adding custom configs for bundlers to import Node.js polyfills. This PR removes that requirement.

Polyfills are required directly in the code. This can be done because the Node.js core modules have the same names (buffer, events, process), and therefore, the code modules are used under Node.js. The polyfills are only imported for browser builds.

This required switching from buffer-es6 to buffer, and process-es6 to process. Both packages are more up-to-date than the ones that have been replaced.

Many lines have been touched, but the lib folder changes are pretty small. Most changes are related to the test code. The same imports are required, but more files are affected.

Besides the globals buffer and process, events is added as a dependency. No code changes are required in that regard because events is not accessible as global, and therefore, require calls are already in the current code.

@mcollina mcollina 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 for this PR! This change would need to be applied to our build/ scripts, not inside lib.

@mcollina

Copy link
Copy Markdown
Member

@ShogunPanda could you review?

Comment thread lib/internal/streams/destroy.js Outdated
Comment thread src/test/browser/fixtures/esbuild-browsers-shims.mjs
@bergos bergos requested a review from ShogunPanda September 19, 2022 08:01
@ShogunPanda

Copy link
Copy Markdown
Contributor

LGTM, if you could address #489 (comment) it would be great.

@ShogunPanda ShogunPanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@bergos bergos requested a review from mcollina September 20, 2022 09:07
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.

3 participants