eslint-plugin-canonical icon indicating copy to clipboard operation
eslint-plugin-canonical copied to clipboard

fix: make rules behave consistently on win32

Open louis-bompart opened this issue 1 year ago • 3 comments

I noticed that multiple rules have a few issues on Windows:

  1. no-barrel-import, virtual-module, and require-extension all assume / is the root of the fs on Windows, which ain't the case.
  2. prefer-import-alias, require-extensions mix up the ECMA path separator and the OS path separator, which are not the same on Windows.
  3. virtual-module mixed up the ECMA path separator and the OS path separator as well, but only in the logging, which can be confusing nonetheless.

https://github.com/gajus/eslint-plugin-canonical/blob/main/src/rules/virtualModule.ts#L29

https://github.com/gajus/eslint-plugin-canonical/blob/main/src/rules/noBarrelImport.ts#L84

To solve that, I propose the following:

  1. In d7880fb, I move the ci setup from a single job to a matrix job that runs on both ubuntu-latest and windows-latest. We can see there that the CI does fail already, despite no changes in the source code.

  2. Make everything works on Windows (todo addcommit sha):

    • Add a util to find the rootPath (so on Windows C:/, or some other letters and on Unix / . Leverage it wherever / was used directly with findDirectory. This fix issue 1
    • Normalize paths using .replaceAll(win32Sep, posixSep). This fixes the two other issues

louis-bompart avatar Feb 02 '24 20:02 louis-bompart

any reason for not merging this one - as currently some of the rules - like virtual-module - do not function properly on windows ?

gabriel-calin-lazar avatar May 02 '24 10:05 gabriel-calin-lazar

Just did not see notifications for this until I had to do a random update.

gajus avatar May 23 '24 19:05 gajus

Just did not see notifications for this until I had to do a random update.

@gajus , I rebased my branch so that it ain't a hassle to review/test, tell me if you need anything ;)

louis-bompart avatar Jun 04 '24 07:06 louis-bompart