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

fix: fix prefix-only modules not marked as builtin #303

Closed
wants to merge 1 commit into from

Conversation

wojtekmaj
Copy link
Contributor

Following @ljharb's feedback in #295 (comment), a different version of isBuiltin function has been implemented.

Copy link

changeset-bot bot commented Jul 16, 2024

🦋 Changeset detected

Latest commit: a63eb46

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-import-resolver-typescript Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codesandbox-ci bot commented Jul 16, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Following @ljharb's feedback in #295 (comment), a different version of isBuiltin function has been implemented.
@ljharb
Copy link
Member

ljharb commented Jul 17, 2024

This will still be a false positive on a node version that lacks the builtin module - node:sqlite before node 22.5, for example.

Copy link
Collaborator

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

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

Agree w/ @ljharb on this one, but for a different reason.

E.g. If I accidentally typo node:slqtie, I'd expect ESLint to help me find out about this.

Block for now, I still prefer porting Node.js implementation:

https://github.com/nodejs/node/blob/e0e0b1a70ebf2b7c6c384771e3f746e9236c4737/lib/internal/bootstrap/realm.js#L314

@SunsetTechuila
Copy link
Contributor

SunsetTechuila commented Jul 17, 2024

Block for now, I still prefer porting Node.js implementation:

it just checks if the module id is in the lists, very similar to what is-core-module does, so there is not really anything to port

https://github.com/nodejs/node/blob/e0e0b1a70ebf2b7c6c384771e3f746e9236c4737/lib/internal/bootstrap/realm.js#L293-L299

@ljharb
Copy link
Member

ljharb commented Jul 17, 2024

@SukkaW the problem is that node doesn't expose which core modules exist at runtime, so to do it without is-core-module (or its approach), you'd need to require.resolve it inside a try/catch or similar, which has a much larger runtime impact than dozens of dependencies, let alone 1-3.

SukkaW added a commit to SukkaW/eslint-import-resolver-typescript that referenced this pull request Jul 22, 2024
@wojtekmaj wojtekmaj closed this Jul 23, 2024
@wojtekmaj wojtekmaj deleted the prefix-only branch July 23, 2024 10:18
github-merge-queue bot pushed a commit that referenced this pull request Jul 23, 2024
* fix(#303): use @nolyfill/is-core-module

* chore: add changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants