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

Update to alex@11 #25

Merged
merged 9 commits into from
Nov 8, 2023
Merged

Conversation

tyler36
Copy link
Contributor

@tyler36 tyler36 commented Nov 7, 2023

Fixes #17

This PR bumps the dependencies required to prevent Dependabot notifications.

@tyler36
Copy link
Contributor Author

tyler36 commented Nov 7, 2023

I tried the changes you suggests but I am still getting errors.
My javascript skills are limited to frontend frameworks so dynamic imports and ES modules are a little over my head.

@tyler36 tyler36 mentioned this pull request Nov 7, 2023
@azu
Copy link
Member

azu commented Nov 7, 2023

Thanks.

Ah, textlint-scripts has a issue.
We want to preserve import(), but it will transpile to require() function.

I've created an issue to upstream textlint/textlint#1283

Edit: fixed in https://github.com/textlint/textlint/releases/tag/v13.4.0

Copy link
Member

@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.

Some packages require Node.js 16+.

Please update CI's node-version.
(Node.js 16 is already End Of Support. So we can put 18+)

-        node-version: [12, 14]
+        node-version: [18, 20]

node-version: [12, 14]

I commented. Probably need to run following command.

yarn add alex
yarn add textlint-scripts --dev
yarn upgrade # need to update lock file

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@tyler36
Copy link
Contributor Author

tyler36 commented Nov 8, 2023

Probably need to run following command.

package.json now includes textlint-scripts so I not sure if we need it.

Now getting error:

Still get ERR_REQUIRE_ESM error when testing locally.

@azu
Copy link
Member

azu commented Nov 8, 2023

package.json now includes textlint-scripts so I not sure if we need it.

Yes, It is required.
We need to update to v13.4.0.

Still get ERR_REQUIRE_ESM error when testing locally.

https://github.com/textlint/textlint/releases/tag/v13.4.0 fix this issue.

yarn add textlint-scripts --dev
yarn upgrade

@azu azu marked this pull request as ready for review November 8, 2023 07:28
.github/workflows/test.yml Outdated Show resolved Hide resolved
@azu
Copy link
Member

azu commented Nov 8, 2023

https://github.com/textlint-rule/textlint-rule-alex/actions/runs/6795156695/job/18472756945?pr=25

/home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/cliui/build/index.cjs:293
const wrap = require('wrap-ansi');
^

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/wrap-ansi/index.js from /home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/cliui/build/index.cjs not supported.
Instead change the require of index.js in /home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/cliui/build/index.cjs to a dynamic import() which is available in all CommonJS modules.
at Object. (/home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/cliui/build/index.cjs:293:14)
at Object. (/home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/yargs/build/index.cjs:2861:12)
at Object. (/home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/yargs/yargs:3:69)
at Object. (/home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/mocha/lib/cli/cli.js:15:15)
at Object. (/home/runner/work/textlint-rule-alex/textlint-rule-alex/node_modules/mocha/bin/mocha.js:141:3) {
code: 'ERR_REQUIRE_ESM'
}

This error caused by old wrap-ansi package.
It will be resolved by run yarn upgrade and commit updated yarn.lock.

📝 yarn upgrade will update the version referenced by the lock file to the latest in the range of semver.

@tyler36
Copy link
Contributor Author

tyler36 commented Nov 8, 2023

Hmmm ... tests passed at e27b519.

@azu
Copy link
Member

azu commented Nov 8, 2023

Hmmm ... tests passed at e27b519.

Yes. This workflow includes yarn upgrade before running tests.
Committing the update to yarn.lock will solve the problem.

yarn upgrade
git add yarn.lock
git commit

@azu
Copy link
Member

azu commented Nov 8, 2023

Fmm. its something wrong.

@azu
Copy link
Member

azu commented Nov 8, 2023

I've fixed in 9f7a8f7

resolutions field fix version forcely.

CI is passed 🎉

Copy link
Member

@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.

LGTM!

Thank you for your patience in proceeding!

@azu azu changed the title Bump dependencies Update to alex@11 Nov 8, 2023
@azu azu merged commit c046a2b into textlint-rule:master Nov 8, 2023
@tyler36
Copy link
Contributor Author

tyler36 commented Nov 8, 2023

9f7a8f7 fixes local testing for me too.

@azu azu added the Type: Dependencies Dependency issues or Changes to dependency files label Nov 8, 2023
@tyler36
Copy link
Contributor Author

tyler36 commented Nov 8, 2023

Yay! 🎊

Thank you very much for your help, guidence and patience.

@tyler36 tyler36 deleted the 20231107_bump_dependencies branch November 8, 2023 08:00
@azu
Copy link
Member

azu commented Nov 8, 2023

@tyler36
Copy link
Contributor Author

tyler36 commented Nov 8, 2023

4.0.0 fixes "Got allows a redirect to a UNIX socket" dependable alert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Dependencies Dependency issues or Changes to dependency files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update alex version
2 participants