Skip to content

feat(allow-modules): include virtual: in the modules pattern #425

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

Merged
merged 2 commits into from
Mar 26, 2025

Conversation

lfantone
Copy link

Update Module Name Pattern to Support Virtual Modules

Description
• Modified the regular expression pattern in the schema definition to allow an optional "virtual:" prefix. This update enables the configuration to recognize virtual modules.
• The updated pattern now supports module identifiers that may start with "virtual:" followed by optionally scoped names and the module name (e.g., "virtual:@scope/module" or "virtual:module").
• Existing validation behavior is preserved for standard module names, ensuring backward compatibility.
• No changes were made to the core logic; only the schema pattern expression has been updated.

Copy link

@vceder vceder left a comment

Choose a reason for hiding this comment

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

❤️

Copy link

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

Can you add some test cases to avoid possible future regressions?

@scagood
Copy link

scagood commented Mar 23, 2025

I have never seen this before, what package manager uses virtual?

@lfantone
Copy link
Author

I have never seen this before, what package manager uses virtual?

Vite has a convention for them https://vite.dev/guide/api-plugin#virtual-modules-convention and there are frameworks and libraries taking advantage of this. One good example is Remix

@lfantone
Copy link
Author

lfantone commented Mar 24, 2025

PR updated with test cases for virtual:

Unsure why is now failing when running test:types, @aladdin-add if you have any clue or maybe is not related to my changes? Saw that this is coming from master

@lfantone lfantone requested a review from aladdin-add March 24, 2025 12:44
@aladdin-add
Copy link

not related to the change - I'll try to fix it later. ❤️

Copy link

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM. Would like @scagood re-review before merging.

@aladdin-add aladdin-add requested a review from scagood March 24, 2025 13:00
Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

Thanks for the info and link!

It would be good if we could add some tests for the other rules that use get-allow-modules (I believe that no-missing-* uses this too?


I swear I fixed those types errors! Sorry about them 😢

@lfantone
Copy link
Author

No worries about the types issues!

I will write test for the other rules 👍

@lfantone lfantone force-pushed the patch-1 branch 2 times, most recently from 8cf2ef2 to d979195 Compare March 25, 2025 08:08
@lfantone lfantone requested a review from scagood March 25, 2025 08:10
Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@aladdin-add aladdin-add merged commit a109793 into eslint-community:master Mar 26, 2025
29 checks passed
@lfantone lfantone deleted the patch-1 branch March 26, 2025 09:42
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