Skip to content

Commit

Permalink
Revert "chore(package): use yarn instead"
Browse files Browse the repository at this point in the history 
This reverts commit dacd62f.
  • Loading branch information
@chemzqm
chemzqm committed Aug 31, 2023
1 parent dacd62f commit 15c8946
Show file tree
Hide file tree
Showing 4 changed files with 7,782 additions and 195 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ jobs:
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node }}
cache: "yarn"
cache: "npm"

- name: Setup python3
uses: actions/setup-python@v4
Expand All @@ -64,7 +64,7 @@ jobs:
- name: Install Dependencies
run: |
yarn global add bytes
yarn install --frozen-lockfile
npm install --frozen-lockfile
sudo apt-get install -y xclip ripgrep exuberant-ctags
xclip -version
rg --version
Expand Down
7 changes: 2 additions & 5 deletions .github/workflows/lint.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,13 @@ jobs:
- name: Setup Node.js
uses: actions/setup-node@v3
with:
cache: "yarn"
cache: "npm"

- name: Install Dependencies
run: yarn install --frozen-lockfile
run: npm install --frozen-lockfile

- name: Check Types by TSC
run: yarn lint:typecheck

- name: Lint ESLint
run: yarn lint

- name: Check Lock File Changes
run: yarn && echo "Listing changed files:" && git diff --name-only --exit-code && echo "No files changed during lint."

4 comments on commit 15c8946

@yaegassy
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @chemzqm, @fannheyward There are currently two lock files, package-lock.json and yarn.lock, present in the master branch. The documentation specifies that installations on the master branch should be done using yarn. Therefore, there might be issues related to installation that could arise, leading to potential issue reports.

@chemzqm
Copy link
Member Author

@chemzqm chemzqm commented on 15c8946 Sep 1, 2023 •
edited

Choose a reason for hiding this comment

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

The test and lint can't run with modules installed by yarn.lock.

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/chemzqm/vim-dev/coc.nvim/node_modules/string-width/index.js from /Users/chemzqm/vim-dev/coc.nvim/node_modules/cliui/build/index.cjs not supported.
Instead change the require of index.js in /Users/chemzqm/vim-dev/coc.nvim/node_modules/cliui/build/index.cjs to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/chemzqm/vim-dev/coc.nvim/node_modules/cliui/build/index.cjs:291:21)
    at Object.<anonymous> (/Users/chemzqm/vim-dev/coc.nvim/node_modules/yargs/build/index.cjs:1:60678)
    at Object.<anonymous> (/Users/chemzqm/vim-dev/coc.nvim/node_modules/yargs/index.cjs:5:3

=============
There was a problem loading formatter: /Users/chemzqm/vim-dev/coc.nvim/node_modules/eslint/lib/cli-engine/formatters/stylish
Error: require() of ES Module /Users/chemzqm/vim-dev/coc.nvim/node_modules/strip-ansi/index.js from /Users/chemzqm/vim-dev/coc.nvim/node_modules/eslint/lib/cli-engine/formatters/stylish.js not supported.
Instead change the require of index.js in /Users/chemzqm/vim-dev/coc.nvim/node_modules/eslint/lib/cli-engine/formatters/stylish.js to a dynamic import() which is available in all CommonJS modules.

Don't know how to fix, the code should be able to be built as expected.

@fannheyward
Copy link
Member

@fannheyward fannheyward commented on 15c8946 Sep 1, 2023 •
edited

Choose a reason for hiding this comment

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

Yes, this is a problem, and I prefer to remove yarn totally.

After we upgrade glob to v10 34f5d96, this uses jackspeak , and cliui, which uses an renamed string-width that broken testing work. isaacs/jackspeak#5

To fix this issue, use an earlier jackspeak, or upgrade to yarn 3. This won't be fixed with yarn 1.

I've tested several ways yesterday:

  1. remove yarn 1, use npm.
    • pros: yarn 1 is dead, long live npm
  2. use glob@9, also supports signal that to fix Suppress scandir access error in /tmp dir #4538
    • pros: no jackspeak, no breaks for anything
    • cons: lock to glob@9
  3. we've upgraded node to v16.18, we can use corepack to use yarn 3: corepack enable; corepack prepare yarn@3.6.3 --activate
    • pros: still use yarn, no more yarn 1 issues
    • cons: needs steps to enable yarn with corepack
  4. I also tested with pnpm because more frontend projects like vue uses this, we can also use corepack to enable this
    • pros: no more yarn 1 issues, more modernize?
    • cons: needs steps to enable it, change our CI, development

Because yarn 1 is dead, I prefer to use npm, remove all yarn scripts.

@yaegassy
Copy link
Contributor

Choose a reason for hiding this comment

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

@chemzqm, @fannheyward Thanks for the explanation.

Please sign in to comment.