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

closes #1293. allows aliases that start with @ to be caught as internal #1294

Merged
merged 1 commit into from Mar 3, 2019

Conversation

Projects
None yet
4 participants
@jeffshaver
Copy link
Contributor

jeffshaver commented Feb 21, 2019

@ljharb as we talked about in the issue, here is the fix. Please let me know if there are any issues. Or if you think we need more tests for this case. I added one. There were already a ton that didn't pass and didn't think I should fix them in this PR.

Fixes #1293. Unblocked by #1295.

@jeffshaver jeffshaver force-pushed the jeffshaver:master branch 3 times, most recently from 91a439c to 3343898 Feb 21, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 21, 2019

Coverage Status

Coverage decreased (-0.5%) to 93.651% when pulling 91a439c on jeffshaver:master into bdc05aa on benmosher:master.

4 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 21, 2019

Coverage Status

Coverage decreased (-0.5%) to 93.651% when pulling 91a439c on jeffshaver:master into bdc05aa on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 21, 2019

Coverage Status

Coverage decreased (-0.5%) to 93.651% when pulling 91a439c on jeffshaver:master into bdc05aa on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 21, 2019

Coverage Status

Coverage decreased (-0.5%) to 93.651% when pulling 91a439c on jeffshaver:master into bdc05aa on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 21, 2019

Coverage Status

Coverage decreased (-0.5%) to 93.651% when pulling 91a439c on jeffshaver:master into bdc05aa on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 21, 2019

Coverage Status

Coverage increased (+3.2%) to 97.317% when pulling 651829d on jeffshaver:master into e9544f8 on benmosher:master.

@jeffshaver jeffshaver force-pushed the jeffshaver:master branch from 3343898 to f2da641 Feb 21, 2019

function baseModule(name) {
if (isScoped(name)) {
function baseModule(name, path) {
if (isScoped(name, path)) {

This comment has been minimized.

@ljharb

ljharb Feb 23, 2019

Collaborator

i don't think it makes sense to make isScoped check isExternalPath; i think all the places currently calling isScoped should be explicitly handling isExternalPath first.

This comment has been minimized.

@jeffshaver

jeffshaver Feb 23, 2019

Author Contributor

I can do that. It does seem that "scoped" packages have a connotation meaning "external". So I thought it would be fine putting it in isScoped because scoped modules have to be external. Will update shortly

This comment has been minimized.

@jeffshaver

jeffshaver Feb 23, 2019

Author Contributor

Actually... I don't even know how to solve this? Move the isScoped check down underneath isInternalModulein the lodash.cond call? The problem I am seeing is that isScoped means external, but isExternalModule doesn't check it. It is checked by the lodash.cond call

This comment has been minimized.

@jeffshaver

jeffshaver Feb 23, 2019

Author Contributor

Seems like moving the internal check above scoped and external solved that problem and let me revert most of the other changes

This comment has been minimized.

@ljharb

ljharb Feb 24, 2019

Collaborator

Scoped packages are the same as anything in node_modules; they're things installed from npm. I don't think "scoped" means "external".

@jeffshaver jeffshaver force-pushed the jeffshaver:master branch from f2da641 to a8e0018 Feb 23, 2019

@jeffshaver

This comment has been minimized.

Copy link
Contributor Author

jeffshaver commented Feb 23, 2019

@ljharb updated per your comments. Appreciate you looking at this!

@ljharb

ljharb approved these changes Feb 24, 2019

Copy link
Collaborator

ljharb left a comment

Seems good; it'll have to wait tho until master gets unbroken.

@jeffshaver

This comment has been minimized.

Copy link
Contributor Author

jeffshaver commented Feb 24, 2019

@ljharb anything I can do to help unbreak master?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 24, 2019

@jeffshaver open another PR that fixes the tests? :-D

@jeffshaver

This comment has been minimized.

Copy link
Contributor Author

jeffshaver commented Feb 24, 2019

@ljharb. I'll take a look later today to see if I can fix some

@jeffshaver jeffshaver force-pushed the jeffshaver:master branch from a8e0018 to ee11348 Mar 3, 2019

@jeffshaver

This comment has been minimized.

Copy link
Contributor Author

jeffshaver commented Mar 3, 2019

@ljharb since my other pr got merged (thanks! sorry we couldn't fix that one test), i rebased my branch with upstream master. just wanted to let you know

@jeffshaver jeffshaver force-pushed the jeffshaver:master branch from ee11348 to 55ee760 Mar 3, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Mar 3, 2019

@jeffshaver oh we fixed it! thanks for the PR, and the ping.

@ljharb

ljharb approved these changes Mar 3, 2019

[isExternalModule, constant('external')],
[isScoped, constant('external')],
[isInternalModule, constant('internal')],

This comment has been minimized.

@ljharb

ljharb Mar 3, 2019

Collaborator

This looks good, but there is the possibility this will be a breaking change - however, since it doesn't break any tests, and since it's unlikely anyone is depending on the inability to mark an external module as "internal", it's probably fine.

@ljharb ljharb force-pushed the jeffshaver:master branch from 55ee760 to 651829d Mar 3, 2019

@ljharb ljharb merged commit 651829d into benmosher:master Mar 3, 2019

2 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeffshaver

This comment has been minimized.

Copy link
Contributor Author

jeffshaver commented Mar 3, 2019

@ljharb appreciate it! When do you think a new version will be released with these changes?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Mar 3, 2019

Probably not super quickly; ping me again in a week if it’s not done by then.

@X9X

This comment has been minimized.

Copy link

X9X commented on 651829d Mar 6, 2019

Great Work! But is there a schedule to publish a patch version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.