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 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 February 7, 2020 10:41
Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@coveralls
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
Copy link

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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

if (normSubpath.length === 0) {
return false
}
path = path.replace(/\\/g, '/')
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@fisker fisker Feb 14, 2020

Choose a reason for hiding this comment

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

I don't think path.normalize works like expected

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

@ljharb
Copy link
Member

ljharb commented Feb 13, 2020

(windows tests are broken pending #1648)

@ljharb
Copy link
Member

ljharb commented Feb 14, 2020

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

@fisker
Copy link
Contributor Author

fisker commented Feb 14, 2020

Should be good now.

@fisker
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.

@raphinesse
Copy link
Contributor

@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
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?

@raphinesse
Copy link
Contributor

@fisker I'm trying right now: 9e89605

@fisker
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
Copy link
Contributor

@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
Copy link
Contributor Author

fisker commented Feb 14, 2020

@raphinesse You're right, problem solved. Thanks

@raphinesse
Copy link
Contributor

@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
Copy link
Member

ljharb commented Feb 14, 2020

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

@fisker
Copy link
Contributor Author

fisker commented Feb 14, 2020

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

@raphinesse
Copy link
Contributor

@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 fix-order branch 6 times, most recently from f73a6c3 to aa6b2a0 Compare February 14, 2020 09:30
@fisker
Copy link
Contributor Author

fisker commented Feb 14, 2020

@raphinesse @ljharb Seems windows tests fixed now.

@ljharb ljharb force-pushed the fix-order branch 2 times, most recently from ca929e3 to 826719e Compare February 14, 2020 18:27
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! 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'],
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ljharb ljharb Feb 16, 2020

Choose a reason for hiding this comment

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

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 :-)

Copy link
Member

Choose a reason for hiding this comment

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

I went ahead and added something like that.

@ljharb
Copy link
Member

ljharb commented Jan 18, 2024

@fisker i'm sorry to hear this was a bad contribution experience for you; I'd love to understand in what way that was the case.

@fisker
Copy link
Contributor Author

fisker commented Jan 18, 2024

This supposed to be a simple fix, but I have to wait/help fix the failed/unrelated tests. If I remember correctly, the failed tests is about coverage on old version of Node.js(at least not something I care at all), and I think it's already failed before I send this. I agree coverage is important, but I don't think it something that important to block. If I'm the maintainer, I'd merge first and try to fix later, especially #1648 already working on it at that time.

I was patient only because I need this fix get released.

It's good to see this plugin still support very old version of ESLint and Node.js, I like write code can run anywhere.
For example, I always use document.{getElements,getElementById} instead of document.querySelector even my target browsers support it, I do this because it's easy to get the same result in one line, but I don't write 10 lines of code when I can do it in one line, I write tools to do the job.

Anyway the contribute experience is really bad to make tests pass on old stuff, because the contributors have to deal with things that they don't care at all.

I stopped look into this repository because I'm an ESLint rule author of many rules, I can simply give up and make my own one. I remember I wrote first rule for prettier internal also because bug in this plugin, and I didn't try to fix it at all.. Update: link

Forgive me to bring this up, no hard feelings, this is just my feeling.

@ljharb
Copy link
Member

ljharb commented Jan 18, 2024

@fisker not at all; i appreciate your willingness to give the feedback even if you've already decided to not contribute in the future. I'll see what I can do in the future to alleviate the "tests for old versions" work for contributors.

(the extensions rule should work with require, fwiw; the config may need commonjs: true though, but if not it's a bug i'd want to fix)

@fisker
Copy link
Contributor Author

fisker commented Jan 18, 2024

I don't remember the details, but I think I speed time to figure but didn't work, so I have to write one. I didn't try because we don't have one at first, now we have a lot, it's much easier than debug existing rules..

@fisker
Copy link
Contributor Author

fisker commented Jan 18, 2024

Reminds me that we don't need import/extensions anymore. 😄

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.

[import/order] 2.20.1 Windows only: Resolver cannot distinguish external and internal imports
4 participants