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

Consider preventing the use of Node.js built-in modules and globals by default #127

Open
Mrtenz opened this issue Oct 13, 2022 · 3 comments

Comments

@Mrtenz
Copy link
Member

Mrtenz commented Oct 13, 2022

Most modules we develop need to be browser-compatible. The use of Node.js built-in modules means that some kind of polyfill is required for those modules. Usually this means using polyfills like crypto-browserify (hasn't been updated in years, and the bundle is quite large), buffer, etc., while there are often better alternatives available that have a much smaller bundle size (e.g., the noble and scure libraries), which offer compatibility with both Node.js and browsers by default.

Working with Uint8Array and bigint instead of Buffer and BN.js is quite easy now, since we have a lot of util functions in @metamask/utils. Most other Buffer-specific functions, like writeUint...() can be replaced with a DataView (available on browsers and Node.js). See MetaMask/key-tree#83 for example.

For that reason I propose we add some ESLint rules that prevent the use of built-in Node.js modules (mainly crypto), and Node.js-specific globals (mainly Buffer).

There are some valid use-cases for using Node.js built-in modules (like Node.js-only modules interacting with the file system), but projects needing it can simply remove or disable those rules.

@Gudahtt
Copy link
Member

Gudahtt commented Oct 13, 2022

Our base ESLint config should already prevent using Node.js or browser-specific APIs (here).

They are allowed for JavaScript files because this template is meant for TypeScript libraries, and what few JavaScript modules we have tend to be build-related, rather than part of the library.

Maybe I'm mistaken though; where have you seen that Node.js APis are allowed where they shouldn't be?

@Mrtenz
Copy link
Member Author

Mrtenz commented Oct 13, 2022

Those rules don't seem to be applied to the module template. As a test I added this file, which does not cause any errors upon running yarn lint.

@mcmire
Copy link
Contributor

mcmire commented Nov 2, 2023

Revisiting this. Does this issue still exist?

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

No branches or pull requests

3 participants