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

fisker · 2020-02-11T09:24:24Z

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

Co-authored-by: Raphael von der Grün <[email protected]> Co-authored-by: Jordan Harband <[email protected]>

coveralls · 2020-02-11T09:46:43Z

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

coveralls · 2020-02-11T09:46:43Z

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

coveralls · 2020-02-11T09:46:43Z

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

coveralls · 2020-02-11T09:46:44Z

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

ljharb · 2020-02-13T05:01:33Z

src/core/importType.js

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


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

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

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

I don't think path.normalize works like expected

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

ljharb · 2020-02-13T05:03:09Z

(windows tests are broken pending #1648)

ljharb · 2020-02-14T06:52:50Z

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

fisker · 2020-02-14T07:29:56Z

Should be good now.

fisker · 2020-02-14T07:46:06Z

@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 · 2020-02-14T08:40:34Z

@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 · 2020-02-14T08:42:35Z

@raphinesse

The error info shows require error

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

raphinesse · 2020-02-14T08:43:40Z

@fisker I'm trying right now: 9e89605

fisker · 2020-02-14T08:44:59Z

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

raphinesse · 2020-02-14T08:49:59Z

@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 · 2020-02-14T08:53:17Z

@raphinesse You're right, problem solved. Thanks

raphinesse · 2020-02-14T08:56:20Z

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

ljharb · 2020-02-14T08:56:57Z

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

fisker · 2020-02-14T09:02:24Z

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

raphinesse · 2020-02-14T09:06:15Z

@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 · 2020-02-14T09:33:55Z

@raphinesse @ljharb Seems windows tests fixed now.

ljharb

Thanks! We're almost there :-)

ljharb · 2020-02-14T18:28:06Z

tests/src/core/importType.js

+  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'],


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?

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

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 · 2020-02-16T07:05:18Z

tests/src/core/importType.js

    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)


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?

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.

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

I went ahead and added something like that.

ljharb · 2024-01-18T18:26:01Z

@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 · 2024-01-18T19:08:41Z

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 · 2024-01-18T19:16:36Z

@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 · 2024-01-18T19:25:11Z

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 · 2024-01-18T19:33:17Z

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

ljharb and others added 2 commits February 7, 2020 10:41

[Tests] appveyor: on node 8-12, use npm 6.10.3

8905007

Co-authored-by: Raphael von der Grün <[email protected]> Co-authored-by: Jordan Harband <[email protected]>

[Fix] order: fix isExternalModule detection on windows

47f912e

fisker mentioned this pull request Feb 11, 2020

Switch from Travis CI to GitHub Actions sindresorhus/eslint-plugin-unicorn#504

Merged

fisker mentioned this pull request Feb 11, 2020

Enable import/order rule sindresorhus/eslint-plugin-unicorn#521

Closed

ljharb force-pushed the fix-order branch from e720ca8 to 615c002 Compare February 13, 2020 05:00

ljharb reviewed Feb 13, 2020

View reviewed changes

fisker mentioned this pull request Feb 14, 2020

Fix build on Windows #1648

Merged

ljharb force-pushed the fix-order branch from 615c002 to d4519f7 Compare February 14, 2020 06:48

fisker force-pushed the fix-order branch from e8eec6b to 1d862f5 Compare February 14, 2020 08:33

fisker force-pushed the fix-order branch from 1d862f5 to a44d67c Compare February 14, 2020 08:40

ljharb force-pushed the fix-order branch from a44d67c to 57713fb Compare February 14, 2020 08:41

fisker force-pushed the fix-order branch from 57713fb to 252c596 Compare February 14, 2020 08:43

fisker force-pushed the fix-order branch from 252c596 to f308211 Compare February 14, 2020 08:56

fisker force-pushed the fix-order branch from f308211 to 591921d Compare February 14, 2020 08:59

fisker force-pushed the fix-order branch 6 times, most recently from f73a6c3 to aa6b2a0 Compare February 14, 2020 09:30

ljharb force-pushed the fix-order branch 2 times, most recently from ca929e3 to 826719e Compare February 14, 2020 18:27

ljharb requested changes Feb 14, 2020

View reviewed changes

ljharb force-pushed the fix-order branch from 826719e to 1b3c8bb Compare February 14, 2020 18:36

ljharb reviewed Feb 16, 2020

View reviewed changes

ljharb force-pushed the fix-order branch from 6930a2f to 47f912e Compare February 17, 2020 06:13

ljharb approved these changes Feb 17, 2020

View reviewed changes

ljharb merged commit 47f912e into import-js:master Feb 17, 2020

fisker deleted the fix-order branch February 17, 2020 06:55

warnellw mentioned this pull request Feb 17, 2020

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

Closed

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

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

Conversation

fisker commented Feb 11, 2020 • edited by ljharb Loading

coveralls commented Feb 11, 2020 • edited Loading

coveralls commented Feb 11, 2020

coveralls commented Feb 11, 2020

coveralls commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

ljharb commented Feb 13, 2020

ljharb commented Feb 14, 2020

fisker commented Feb 14, 2020

fisker commented Feb 14, 2020 • edited Loading

raphinesse commented Feb 14, 2020

fisker commented Feb 14, 2020

raphinesse commented Feb 14, 2020

fisker commented Feb 14, 2020

raphinesse commented Feb 14, 2020

fisker commented Feb 14, 2020

raphinesse commented Feb 14, 2020

ljharb commented Feb 14, 2020

fisker commented Feb 14, 2020

raphinesse commented Feb 14, 2020

fisker commented Feb 14, 2020

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Feb 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jan 18, 2024

fisker commented Jan 18, 2024 • edited Loading

ljharb commented Jan 18, 2024

fisker commented Jan 18, 2024

fisker commented Jan 18, 2024

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

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

fisker commented Feb 11, 2020 •

edited by ljharb

Loading

coveralls commented Feb 11, 2020 •

edited

Loading

fisker Feb 14, 2020 •

edited

Loading

fisker commented Feb 14, 2020 •

edited

Loading

ljharb Feb 16, 2020 •

edited

Loading

fisker commented Jan 18, 2024 •

edited

Loading