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 isBuiltIn and builtinModules #2662

Closed
wants to merge 1 commit into from
Closed

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Sep 5, 2024

Following this discussion this PR removes .isBuiltin and .builtinModules.

/cc @jasnell @pi0 @petebacondarwin @IgorMinar

@vicb vicb requested review from a team as code owners September 5, 2024 08:26
@vicb vicb requested review from erikcorry and a-robinson September 5, 2024 08:26
Copy link

github-actions bot commented Sep 5, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@vicb
Copy link
Contributor Author

vicb commented Sep 5, 2024

I have read the CLA Document and I hereby sign the CLA

@vicb
Copy link
Contributor Author

vicb commented Sep 5, 2024

Not sure how to format the code, it would be a nice addition to CONTRIBUTING.md

@petebacondarwin
Copy link
Contributor

Not sure how to format the code, it would be a nice addition to CONTRIBUTING.md

Looks you just need to run

python3 ./tools/cross/format.py

In the CI it runs this with --check.

github-actions bot added a commit that referenced this pull request Sep 5, 2024
@jasnell
Copy link
Member

jasnell commented Sep 5, 2024

I think we missed the window to do a revert on this. These APIs ended up getting pulled into an internal workers release yesterday that is already underweigh.

@vicb
Copy link
Contributor Author

vicb commented Sep 5, 2024 via email

@jasnell
Copy link
Member

jasnell commented Sep 6, 2024

Closing as these have already gone out into a release.

@jasnell jasnell closed this Sep 6, 2024
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