Skip to content

Conversation

@wojtekmaj
Copy link
Contributor

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

@changeset-bot
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

@codesandbox-ci
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