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

Remove requirement for bundler custom config #489

Merged
merged 3 commits into from Sep 20, 2022
Merged

Conversation

bergos
Copy link
Contributor

@bergos bergos commented Sep 14, 2022

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.

Copy link
Member

@mcollina mcollina left a comment

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
Member

@ShogunPanda could you review?

@ShogunPanda
Copy link
Contributor

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

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

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

3 participants