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.

Sign up for GitHub

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

Jump to bottom

Fix @/abc being treated as scoped module #1854

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Fix @/abc being treated as scoped module #1854

merged 1 commit into from
Jul 14, 2020

Conversation

sethidden
Copy link

@sethidden sethidden commented Jul 12, 2020 •
edited
Loading

Fixes #1851

Before this commit, @/abc was being wrongly detected as a scoped module.
This was a problem when somebody had a webpack alias @. (eg. @ -> ./src)

In general, scoped modules can't start with @/, I think they have to be like @abcd/sometext.

@ljharb
[Fix] extensions/importType: Fix @/abc being treated as scoped module
3e65a70 
Fixes #1851

Before this commit, @/abc was being wrongly detected as a scoped module.
This was a problem when somebody had a webpack alias `@`. (eg. @ -> ./src)

In general, scoped modules can't start with @/, I think they have to be like @/abcd.
@sethidden
Copy link
Author

sethidden commented Jul 12, 2020 •
edited
Loading

This function is used only once in the whole codebase, but I guess it could be made less fragile by adding things like:

  • isScopedModule('@abc/') should return false (ie. check if there's content after the slash).

@coveralls
Copy link

coveralls commented Jul 12, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 97.857% when pulling 3e65a70 on 3nuc:master into 843055c on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.857% when pulling abe72f8 on 3nuc:master into 843055c on benmosher:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.857% when pulling abe72f8 on 3nuc:master into 843055c on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.857% when pulling abe72f8 on 3nuc:master into 843055c on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.857% when pulling abe72f8 on 3nuc:master into 843055c on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.857% when pulling abe72f8 on 3nuc:master into 843055c on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.857% when pulling abe72f8 on 3nuc:master into 843055c on benmosher:master.

ljharb
ljharb requested changes Jul 12, 2020
View reviewed changes
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.

Thanks! Would you mind adding a regression test?

@wenfangdu
Copy link
Contributor

@3nuc You are awesome!

ljharb
ljharb approved these changes Jul 14, 2020
View reviewed changes
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.

Beautiful, thanks!

@ljharb ljharb merged commit 3e65a70 into import-js:master Jul 14, 2020
@giwan-dev giwan-dev mentioned this pull request Aug 31, 2020
Closed
@wenfangdu wenfangdu mentioned this pull request Oct 14, 2020
Merged
This was referenced Mar 12, 2021
Closed
Closed
Merged
Merged
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Webpack alias not working
5 participants
@sethidden @coveralls @wenfangdu @ljharb @alexandre-le-borgne