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

[Fix] `order`: fix `isExternalModule` detect on windows #1651

Merged
merged 2 commits into from Feb 17, 2020

Conversation

@fisker
Copy link
Contributor

fisker commented Feb 11, 2020

isExternalModule is not right, when use \ as separator.

This cause external module detected as internal.

Seems we are not running test on windows. I add a test on isExternalModule

Fixes #1655

ljharb and others added 2 commits Feb 7, 2020
Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 11, 2020

Coverage Status

Coverage increased (+0.001%) to 97.839% when pulling 47f912e on fisker:fix-order into 2beec94 on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 11, 2020

Coverage Status

Coverage increased (+0.001%) to 97.839% when pulling e720ca8 on fisker:fix-order into 45f0860 on benmosher:master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 11, 2020

Coverage Status

Coverage increased (+0.001%) to 97.839% when pulling e720ca8 on fisker:fix-order into 45f0860 on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 11, 2020

Coverage Status

Coverage increased (+0.001%) to 97.839% when pulling e720ca8 on fisker:fix-order into 45f0860 on benmosher:master.

@ljharb ljharb force-pushed the fisker:fix-order branch from e720ca8 to 615c002 Feb 13, 2020
if (normSubpath.length === 0) {
return false
}
path = path.replace(/\\/g, '/')

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 13, 2020

Collaborator

perhaps instead of handling both cases, we should use path.sep, so that it handles file paths according to the filesystem you're on?

This comment has been minimized.

Copy link
@fisker

fisker Feb 13, 2020

Author Contributor

I'm not sure that if path.sep works, but I guess no harm we just simply normalize the path?

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 14, 2020

Collaborator

in that case, would node's path.normalize work?

This comment has been minimized.

Copy link
@fisker

fisker Feb 14, 2020

Author Contributor

I don't think path.normalize works like expected

require('path').normalize('E:/path\\to/')
'E:\\path\\to\\'
@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 13, 2020

(windows tests are broken pending #1648)

@fisker fisker mentioned this pull request Feb 14, 2020
@ljharb ljharb force-pushed the fisker:fix-order branch from 615c002 to d4519f7 Feb 14, 2020
@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 14, 2020

Rebased this on top of #1648; still 1 test failing.

@fisker

This comment has been minimized.

Copy link
Contributor Author

fisker commented Feb 14, 2020

Should be good now.

@fisker

This comment has been minimized.

Copy link
Contributor Author

fisker commented Feb 14, 2020

@ljharb I fixed the failed test. But seems still breaking on Node.js@10. But local test passed on my laptop.... This has to be something related to require.resolve bug on Node.js@10, I'll figure.

@fisker fisker force-pushed the fisker:fix-order branch from e8eec6b to 1d862f5 Feb 14, 2020
@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Feb 14, 2020

@fisker The failures on Node 10 are due to the same problem with nyc that we did work around for in #1648 for Node 12 only.

@fisker fisker force-pushed the fisker:fix-order branch from 1d862f5 to a44d67c Feb 14, 2020
@ljharb ljharb force-pushed the fisker:fix-order branch from a44d67c to 57713fb Feb 14, 2020
@fisker

This comment has been minimized.

Copy link
Contributor Author

fisker commented Feb 14, 2020

@raphinesse

The error info shows require error

If we use that code for node.js 10, will it work?

@fisker fisker force-pushed the fisker:fix-order branch from 57713fb to 252c596 Feb 14, 2020
@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Feb 14, 2020

@fisker I'm trying right now: 9e89605

@fisker

This comment has been minimized.

Copy link
Contributor Author

fisker commented Feb 14, 2020

@raphinesse If that's a bug from nyc, why not disable coverage on windows? Should be simplier

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Feb 14, 2020

@raphinesse If that's a bug from nyc, why not disable coverage on windows? Should be simplier

@fisker I suggested that in #1648 (comment) but that comment got no reply from @ljharb. So I cannot tell you why.

@fisker

This comment has been minimized.

Copy link
Contributor Author

fisker commented Feb 14, 2020

@raphinesse You're right, problem solved. Thanks

@fisker fisker force-pushed the fisker:fix-order branch from 252c596 to f308211 Feb 14, 2020
@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Feb 14, 2020

@fisker The problem remains on Node 8. I've updated #1648 to use npm@6.10.3 for all Node versions. We'll see if that works for Node 8 too.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 14, 2020

I have no interest in disabling coverage anywhere; anything that disables coverage on windows is a nonstarter.

@fisker fisker force-pushed the fisker:fix-order branch from f308211 to 591921d Feb 14, 2020
@fisker

This comment has been minimized.

Copy link
Contributor Author

fisker commented Feb 14, 2020

@raphinesse Have you try c8 on windows? Maybe that works.

@raphinesse

This comment has been minimized.

Copy link
Contributor

raphinesse commented Feb 14, 2020

@fisker Sorry, I don't have time to do so right now. Plus I fear it might not support enough Node/npm versions. That's just speculation though.

@fisker fisker force-pushed the fisker:fix-order branch 6 times, most recently from f73a6c3 to aa6b2a0 Feb 14, 2020
@fisker

This comment has been minimized.

Copy link
Contributor Author

fisker commented Feb 14, 2020

@raphinesse @ljharb Seems windows tests fixed now.

@ljharb ljharb force-pushed the fisker:fix-order branch 2 times, most recently from ca929e3 to 826719e Feb 14, 2020
Copy link
Collaborator

ljharb left a comment

Thanks! We're almost there :-)

it('`isExternalModule` works with windows directory separator', function() {
expect(isExternalModule('foo', {}, 'E:\\path\\to\\node_modules\\foo')).to.equal(true)
expect(isExternalModule('foo', {
'import/external-module-folders': ['E:\\path\\to\\node_modules'],

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 14, 2020

Collaborator

it doesn't seem like we're testing external-module-folders here if it was true before? meaning, maybe we need to add a test that returns false without external-module-folders, and true with it?

This comment has been minimized.

Copy link
@fisker

fisker Feb 14, 2020

Author Contributor

No, without external-module-folders still true, because subpath defualts to node_modules, the test make sure we need replace subpath, remove that line will cause error

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 14, 2020

Collaborator

Sorry, to clarify; I'm glad these tests fail without your fix! However, don't we want to also test that a windows path would not be external without the right external-module-folders setting?

@ljharb ljharb force-pushed the fisker:fix-order branch from 826719e to 1b3c8bb Feb 14, 2020
expect(isExternalModule('foo', {
'import/external-module-folders': ['E:\\path\\to\\node_modules'],
}, 'E:\\path\\to\\node_modules\\foo')).to.equal(true)
expect(isExternalModule('foo', {
'import/external-module-folders': ['E:\\path\\to\\node_modules'],
}, '.\\foo')).to.equal(false)

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 16, 2020

Collaborator

i'm hoping for "false before, true after", not "same before and after". Can we come up with an example where the external-module-folders setting changes the result?

This comment has been minimized.

Copy link
@fisker

fisker Feb 16, 2020

Author Contributor

I'm not sure I understand this.

Setting import/external-module-folders did not change the result.

    expect(isExternalModule('foo', {
      'import/external-module-folders': ['E:\\path\\to\\node_modules'],
    }, 'E:\\path\\to\\node_modules\\foo')).to.equal(true)
    expect(isExternalModule('foo', {
    }, 'E:\\path\\to\\node_modules\\foo')).to.equal(true)

There are all true, and should be true, but before I add subpath.replace(/\\/g, '/'), this is false on windows, because it's bug. How do I test that, if you want reproduction, you just remove this fix, and

    expect(isExternalModule('foo', {
      'import/external-module-folders': ['E:\\path\\to\\node_modules'],
    }, 'E:\\path\\to\\node_modules\\foo')).to.equal(true)

this will fail.

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 16, 2020

Collaborator

sure - i appreciate that the bug is that you get true, false instead of true, true.

However, to prevent different regressions later, i'm hoping to also have a test that correctly ensures false, true, even if that test would pass on master now :-)

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 17, 2020

Collaborator

I went ahead and added something like that.

@ljharb ljharb force-pushed the fisker:fix-order branch from 6930a2f to 47f912e Feb 17, 2020
@ljharb
ljharb approved these changes Feb 17, 2020
@ljharb ljharb merged commit 47f912e into benmosher:master Feb 17, 2020
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 97.839%
Details
@fisker fisker deleted the fisker:fix-order branch Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.