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

Linux-style environment variables in npm scripts are not supported on Windows #189

Open
yasuking0304 opened this issue Mar 27, 2024 · 9 comments
Assignees

Comments

@yasuking0304
Copy link
Contributor

yasuking0304 commented Mar 27, 2024

Problem

Currently, when I run test in a Windows environment, it fails.

Suggestion

If you introduce "cross-env", you will be able to execute test independently of the OS.

now
package.json

  "scripts": {
    "test": "npm run build && NODE_OPTIONS=--experimental-vm-modules jest",

...

  "devDependencies": {
    "husky": "^8.0.3",
    "lint-staged": "^13.1.2"
  }

after

  "scripts": {
    "test": "npm run build && cross-env NODE_OPTIONS=--experimental-vm-modules jest",

...

  "devDependencies": {
    "cross-env": "^5.0.5",
    "husky": "^8.0.3",
    "lint-staged": "^13.1.2"
  }
@xicri
Copy link
Owner

xicri commented Mar 27, 2024

  • Because cross-env is no longer maintained and its GitHub repository is already archived, I want to avoid adding it if possible.
  • I recently prefer Vitest to Jest for better ESM support. Because NODE_OPTIONS=--experimental-vm-modules is required to support ESM on Jest, you would not need environment variables in the npm script by migrating to Vitest.
  • Since supporting Windows as a development environment is a bit hard, I recommend using WSL.

So this time, I want to solve this problem by migrating to Vitest or using WSL.

@xicri xicri changed the title [Enhancement] I want to pass test on Windows environment as well Linux-style environment variables in npm script is not supported on Windows Mar 27, 2024
@xicri xicri changed the title Linux-style environment variables in npm script is not supported on Windows Linux-style environment variables in npm scripts are not supported on Windows Mar 27, 2024
@xicri
Copy link
Owner

xicri commented Mar 27, 2024

Closing for now.

@xicri xicri closed this as completed Mar 27, 2024
@xicri
Copy link
Owner

xicri commented May 14, 2024

FYI. I have migrated from Jest to Vitest.
Now the environment variable is removed from npm scripts and they should not cause syntax errors in most cases.

@xicri
Copy link
Owner

xicri commented Sep 19, 2024

Does this issue still affect on your end?
I reminded WSL might unavailable on some edition(s) of Windows, while the google results says it is no longer true. Does your Windows support WSL?

Even if WSL is available on your end, I have noticed it might be a bit troublesome for Windows-based developers to set up WSL just for npm scripts.

I still use find command in npm run lint to validate JSON5, so it probably does not work on Windows.

I recently consider running npm scripts on another shell called NuShell.
It is cross-platform and supports bash-like syntax and commands.
It can be installed via npm, so you don't have to manually install it. You just have to run npm install.

Do you think it works for you?
I know you are not familiar with NuShell since it is not so popular, so I will ask you to check if it works before merging this change to main.

@xicri xicri reopened this Sep 19, 2024
@yasuking0304
Copy link
Contributor Author

Thank you for your response.
I have confirmed that npm test works without any problems on Windows.

@xicri
Copy link
Owner

xicri commented Sep 19, 2024

My concern was npm run lint which probably does not work on Windows.

Do you run lint on your local machine? If you don't, I guess I don't have to fix this issue for now.
GitHub Actions also runs lint when you create and update your PRs, so running lint on your local machine is not necessary.

@yasuking0304
Copy link
Contributor Author

Currently, eslint does not work unless you install WSL.
If you do not install WSL, you will have to add code that runs on Windows.

"lint_windows": "(for /r %i in (*.json5) do (json5 %i --validate)) & eslint --ext .js,.mjs,.cjs,.ts,.json,.json5 --ignore-path .gitignore .",

Also, if "git config core.autocrlf" is set to true, you will need to remove the following line from .editorconfig.

end_of_line = lf

Screen capture
WS000038

I run the tests all the time.
This is the evidence of the execution result.

Screen capture
WS000039

console log

実行するタスク: npm run test 


> test
> npm run build && vitest run


> build
> node scripts/build.js

Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/artifacts.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/characters.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/dialogue.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/domains.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/drops-boss.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/drops.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/enemies.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/facilities.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/foods.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/gemstones.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/items.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/living-beings.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/locations.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/objects.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/organizations.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/quests.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/sereniteapot.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/specialties.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/story.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/system.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/talent-materials.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/tmp.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/weapon-materials.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/weapons.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/y-events.json5
Loading D:\Documents\github\genshin-langdata2\dataset\dictionary/z-archives.json5
[UPDATED] パカル (Pacal)
[UPDATED] ワイナ (Wayna)
[NEW] アミナ (Amina)
[UPDATED] フニ (Huni)
[UPDATED] トバ (Toba)
[UPDATED] 山下 (Sanka)
[UPDATED] トリニダード (Trinidad)
[NEW] ティアゴ (Thiago)
[NEW] ポンセ (Ponche)
[NEW] ブルキナ (Burkina)
[NEW] コンガマトー (Kongamato)
[NEW] ポニニ (Ponini)
[NEW] ポギギ (Poggi)
[NEW] ポチチ (Pocchi)
[NEW] ナンナ (Nanna)
[UPDATED] きっちりチップス (Impeccably Organized)
[UPDATED] 幸運のおすそわけ (Pass the Luck)
[NEW] 竜狩り人の褒賞 (Saurian Hunter's Reward)
[UPDATED] クイールとウククの物語 (The Tale of Qoyllor and Ukuku)
[NEW] マーウェと幻写霊 (Maawe and Monetto)
[NEW] 白姫と六人の小人 (The Page Princess and the Six Pygmies)
[NEW] 流泉の帰す場所 (Where the Springs Return)
[NEW] ユパンキの廻焔 (Yupanqui's Turnfire)
[NEW] 祝福を祈り、テペトルに告ぐ (A Prayer for Blessings, Told to Crested Peaks)
[NEW] えっ?ナタ? (Hmm? Natlan?)
[NEW] 手に妙影 (Sharpshooter on Hand)
[NEW] 廻焔 (TurnFire)
[NEW] 山の王の長牙 (Fang of the Mountain King)

 RUN  v2.1.1 D:/Documents/github/genshin-langdata2

 ✓ tests/dictionary.test.js (2) 493ms
 ✓ tests/json5.test.js (6) 4084ms
 ✓ tests/tags.test.js (1)
 ✓ tests/utils.test.js (1)

 Test Files  4 passed (4)
      Tests  10 passed (10)
   Start at  19:22:20
   Duration  5.62s (transform 764ms, setup 0ms, collect 3.55s, tests 4.60s, environment 1ms, prepare 875ms)

 *  ターミナルはタスクで再利用されます、閉じるには任意のキーを押してください。 

@xicri
Copy link
Owner

xicri commented Sep 20, 2024

Thanks, looks like a Windows-specific npm script is required to lint on Windows as expected...

I'm also considering to convert JSON5s to TypeScript. By the migration to TypeScript, find command can be removed, so maybe migration to NuShell is not required if I migrate to TypeScript.

Also, if "git config core.autocrlf" is set to true, you will need to remove the following line from .editorconfig.

end_of_line = lf

Oops, I didn't expect such problem.
The problem might be resolved by adding .gitattributes.

I usually add .gitattributes as follows so that Git converts any crlf linebreaks to lf on the commit:

* text=auto eol=lf

By adding .gitattributes, you might no longer have to remove end_of_line = lf from .editorconfig.
I'm not sure if it automatically disables core.autocrlf for genshin-langdata repo, but I think it is worth trying.

I just noticed .gitattributes is not in the genshin-langdata repo, so I will add it later.
Let me know the results after I added it.

@xicri
Copy link
Owner

xicri commented Sep 20, 2024

I have separated the autocrlf issue to #302.

@xicri xicri self-assigned this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants