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: not working on Windows 10 #21

Merged
merged 7 commits into from
Feb 3, 2024
Merged

fix: not working on Windows 10 #21

merged 7 commits into from
Feb 3, 2024

Conversation

starnayuta
Copy link
Contributor

@starnayuta starnayuta commented Feb 3, 2024

Change the command for running eslint from node.js to npx for compatibility with Windows.

Closes #20

@starnayuta
Copy link
Contributor Author

The test results are as follows.
I could not determine if this result passed the test.

PS C:\path\to\eslint-cjs-to-esm> node eslint-cjs-to-esm.js "./test-fixtures/*"  

C:\path\to\eslint-cjs-to-esm\test-fixtures\import-index.ts
  1:26  error  require file extension '/index.js'  file-extension-in-import-ts/file-extension-in-import-ts
C:\path\to\eslint-cjs-to-esm\test-fixtures\import-no-js-suffix.ts
  1:8  error  require file extension '.js'  file-extension-in-import-ts/file-extension-in-import-ts
  1:8  error  require file extension '.ts'  node/file-extension-in-import

C:\path\to\eslint-cjs-to-esm\test-fixtures\no-node-protocol.ts
  1:18  error  Prefer `node:path` over `path`  unicorn/prefer-node-protocol

C:\path\to\eslint-cjs-to-esm\test-fixtures\require.ts
  1:1  error  Expected "import" instead of "require()"  import/no-commonjs
  1:1  error  Do not use "require"                      unicorn/prefer-module

✖ 6 problems (6 errors, 0 warnings)
  4 errors and 0 warnings potentially fixable with the `--fix` option.

PS C:\path\to\eslint-cjs-to-esm> node eslint-cjs-to-esm.js "./test-fixtures/*" --fix

C:\path\to\eslint-cjs-to-esm\test-fixtures\require.ts
  1:1  error  Expected "import" instead of "require()"  import/no-commonjs
  1:1  error  Do not use "require"                      unicorn/prefer-module

✖ 2 problems (2 errors, 0 warnings)

Copy link
Owner

@azu azu left a comment

Choose a reason for hiding this comment

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

jobs:
test:
name: "Test on Node.js ${{ matrix.node-version }}"
runs-on: ubuntu-latest
strategy:
matrix:
node-version: [ 18,20 ]
steps:
- name: checkout
uses: actions/checkout@v4
- name: setup Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v4
with:
cache: "npm"
node-version: ${{ matrix.node-version }}
- name: Install
run: npm ci
- name: Test
run: npm test

Can you add a matrix?

  test:
    name: Test(Node ${{ matrix.node }} on ${{ matrix.os }})
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [ubuntu-latest, windows-latest]
        node: [ 18, 20 ]

eslint-cjs-to-esm.js Outdated Show resolved Hide resolved
@azu
Copy link
Owner

azu commented Feb 3, 2024

https://github.com/azu/eslint-cjs-to-esm/blob/main/package.json#L40
This test confirms that the exit status is 1

@azu
Copy link
Owner

azu commented Feb 3, 2024

Another option:

if process are Windows, just add .cmd like this.
https://github.com/saltyshiomix/resolve-as-bin/blob/23908b1c46827f6fd41cdcc401d3d2f5af76eade/index.ts#L13

@starnayuta
Copy link
Contributor Author

Great. I will try this approach.

Thank you.

Another option:

if process are Windows, just add .cmd like this. https://github.com/saltyshiomix/resolve-as-bin/blob/23908b1c46827f6fd41cdcc401d3d2f5af76eade/index.ts#L13

@starnayuta starnayuta changed the title BREAKING CHANGE: Changed from running eslint via node to npm. fix: not working on Windows 10 Feb 3, 2024
@starnayuta
Copy link
Contributor Author

@azu

I changed the implementation and test matrix in the way you explained.

The behaviour is now not changed except in Windows.

@starnayuta
Copy link
Contributor Author

This error also happened on my local PC.
I am not sure what caused the error, but I will try to check.

https://github.com/azu/eslint-cjs-to-esm/actions/runs/7765836291/job/21181030640?pr=21

@azu
Copy link
Owner

azu commented Feb 3, 2024

https://github.com/azu/eslint-cjs-to-esm/actions/runs/7765836291/job/21181030640?pr=21
Ah, This may be expected-exit-status's issue.

We need to run eslint-cjs-to-esm by another way.

@azu
Copy link
Owner

azu commented Feb 3, 2024

''node' is not recognized as an internal or external command,

Probably, need to use node.exe or node.cmd in Windows?

@starnayuta
Copy link
Contributor Author

starnayuta commented Feb 3, 2024

In cmd, single quote ' seem to be treated as just characters in most cases.
It was changed to double quote ".

https://stackoverflow.com/questions/51080215/differences-between-single-and-double-quotes-in-cmd

@azu
Copy link
Owner

azu commented Feb 3, 2024

https://github.com/azu/eslint-cjs-to-esm/actions/runs/7765962449/job/21181296571?pr=21
work on macOS/linux by following change:

- ./test-fixtures/*
+ ./test-fixtures/*.ts

@azu azu added the Type: Bug Bug or Bug fixes label Feb 3, 2024
@azu azu merged commit 4152c34 into azu:main Feb 3, 2024
4 checks passed
@starnayuta
Copy link
Contributor Author

Thank you for your kind assistance.

@azu
Copy link
Owner

azu commented Feb 3, 2024

Thanks for work!

Release
https://github.com/azu/eslint-cjs-to-esm/releases/tag/v3.0.1 🎉

@starnayuta starnayuta deleted the fix-for-windows branch February 5, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug or Bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not working on Windows 10
2 participants