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

[Tests] extensions: add cases to verify @/configs/chart is treated as a package #1925

Merged
merged 1 commit into from Feb 1, 2021

Conversation

wenfangdu
Copy link
Contributor

@wenfangdu wenfangdu commented Oct 14, 2020

Fixes #1851

Before this fix, 'import/extensions': ['warn', 'ignorePackages'] wouldn't warn import chart from '@/configs/chart', this is an improvement over PR #1854.

@coveralls
Copy link

coveralls commented Oct 14, 2020

Coverage Status

Coverage increased (+0.4%) to 71.388% when pulling 1031e1c on wenfangdu:master into 4c92c47 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.04%) to 95.86% when pulling 873cfce on wenfangdu:master into a00727e on benmosher:master.

@wenfangdu wenfangdu changed the title fix: fixes #1851 fix(rules/extensions): fix @/configs/chart being treated as a package. Oct 14, 2020
@@ -367,6 +368,11 @@ ruleTester.run('extensions', rule, {
line: 4,
column: 31,
},
{
message: 'Missing file extension for "@/configs/chart"',
Copy link
Member

Choose a reason for hiding this comment

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

again tho, this should never have a warning when ignorePackages is set, because it's a bare idenfier - a package.

Copy link
Contributor Author

@wenfangdu wenfangdu Oct 14, 2020

Choose a reason for hiding this comment

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

According to the scoped packages doc, @somescope/somepackagename is a scoped package, whereas @/foo/bar is not. Publishing @/foo/bar as an unscoped package is also not allowed, so it's neither a scoped package nor an unscoped package, therefore not a package.
image

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it isn't "a scoped package", but since it doesn't start with . or /, it is a "bare identifier", which is always "a package" - packages aren't just "publishable packages", they're "anything with a bare identifier".

Copy link
Member

Choose a reason for hiding this comment

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

Circling back on this - @ is a package here (just not a publishable one), so I think this should be ignored when ignorePackages is enabled.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I've rebased this to only include updated test cases, since this is already working as intended.

@@ -367,6 +368,11 @@ ruleTester.run('extensions', rule, {
line: 4,
column: 31,
},
{
message: 'Missing file extension for "@/configs/chart"',
Copy link
Member

Choose a reason for hiding this comment

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

Circling back on this - @ is a package here (just not a publishable one), so I think this should be ignored when ignorePackages is enabled.

@ljharb ljharb changed the title fix(rules/extensions): fix @/configs/chart being treated as a package. [Tests] extensions: add cases to verify @/configs/chart is treated as a package Feb 1, 2021
@ljharb ljharb merged commit 1031e1c into import-js:master Feb 1, 2021
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.

Webpack alias not working
3 participants