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

deps: replace builtins dependency with Node.js module.builtinModules #104

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

thecodrr
Copy link
Contributor

This PR replaces the builtins dependency with the Node.js internal module.builtinModules property/constant. This change does not break any functionality of this package.

I am aware of this comment in the CONTRIBUTING.md file:

It should be noted that our team does not accept third-party dependency updates/PRs. If you submit a PR trying to update our dependencies we will close it with or without a reference to these contribution guidelines.

In my defense, this PR does not add a third-party dependency but removes one so I think it should be considered.

References

@thecodrr thecodrr requested a review from a team as a code owner December 13, 2023 13:25
@wraithgar
Copy link
Member

  • module.builtinModules was added in v9.3.0, v8.10.0, v6.13.0 so we are good on the engines.
  • module.isBuiltin wasn't added till v18.6.0, v16.17.0 so we can't use that.
  • node:module did not exist in v14.17.0 so we can't use that.

@wraithgar wraithgar changed the title Replace builtins dependency with Node.js module.builtinModules deps: replace builtins dependency with Node.js module.builtinModules Dec 13, 2023
@wraithgar wraithgar merged commit f12f849 into npm:main Dec 13, 2023
27 checks passed
@github-actions github-actions bot mentioned this pull request Dec 13, 2023
@thecodrr thecodrr deleted the remove-builtins-dep branch December 13, 2023 16:47
@github-actions github-actions bot mentioned this pull request May 3, 2024
lukekarrys pushed a commit that referenced this pull request May 6, 2024
🤖 I have created a release *beep* *boop*
---


##
[5.0.1](v5.0.0...v5.0.1)
(2024-05-06)

### Dependencies

*
[`f12f849`](f12f849)
[#104](#104)
replace `builtins` dependency with Node.js `module.builtinModules`
(#104)

### Chores

*
[`f2b3233`](f2b3233)
[#112](#112) auto
publish (#112) (@lukekarrys)
*
[`406b31a`](406b31a)
[#110](#110) bump
@npmcli/template-oss to 4.22.0 (@lukekarrys)
*
[`bcc451a`](bcc451a)
[#69](#69) update
tap coverage in package.json (#69) (@wraithgar)
*
[`320e5dd`](320e5dd)
[#68](#68) add new
tests to reach full test coverage (#68) (@janbritz)
*
[`5c72411`](5c72411)
[#59](#59) bump
@npmcli/eslint-config from 3.1.0 to 4.0.0 (@dependabot[bot])
*
[`a893e39`](a893e39)
[#110](#110)
postinstall for dependabot template-oss PR (@lukekarrys)
*
[`13f9b85`](13f9b85)
[#109](#109) bump
@npmcli/template-oss from 4.21.3 to 4.21.4 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@vikiboss
Copy link

It appears that this PR introduces a breaking change in functionality.

In version 5.0.0, the builtins package was employed to identify built-in package names. This package is environment-agnostic and seamlessly operates within browser environments. However, starting with version 5.0.1, the implementation shifted to leveraging Node.js's internal module to perform this detection. This change inherently disrupts compatibility in browser contexts.

Projects upgrading to version 5.0.1 may encounter issues, even though build tools such as Webpack might offer superficial polyfills for these packages. Notably, the invocation of the includes method on builtins leads to errors, diverging from its intended behavior and inadvertently breaking functionality.

I recommend revisiting this implementation to ensure cross-environment compatibility and preserving the non-breaking nature of updates.

@boda-sh
Copy link

boda-sh commented Aug 18, 2024

Agreed with @vikiboss that a environment-agnostic solution might be preferred, see #126

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.

4 participants