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

Use ES modules for gatsby config #1429

Merged
merged 3 commits into from
Jul 7, 2023
Merged

Use ES modules for gatsby config #1429

merged 3 commits into from
Jul 7, 2023

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Jun 17, 2023

This should unlock some dependency updates

@zbynek zbynek requested a review from a team as a code owner June 17, 2023 17:25
@zbynek zbynek marked this pull request as draft June 17, 2023 17:38
@zbynek zbynek marked this pull request as draft June 17, 2023 17:38
@halkeye
Copy link
Member

halkeye commented Jun 17, 2023

this is so cool. I wasn't aware we upgraded gatsby enough to support this.

@zbynek zbynek force-pushed the esm branch 3 times, most recently from 5f66733 to 0e7d438 Compare June 18, 2023 23:11
@zbynek
Copy link
Contributor Author

zbynek commented Jun 19, 2023

@halkeye there are still some files that we might be able to rename, but when I tried that with ucfirst I ran into some issues with Eslint. Also Eslint says p-queue is not reachable even though it clearly is. Maybe because type:module in package.json is expected by Eslint but causes problems with Gatsby? (for plugin-site; other workspaces are fine). Despite these I think this is ready for merge, we can improve it more when newer versions of Eslint and/or Gatsby are available.

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Awesome, thanks so much!

@@ -64,6 +64,7 @@ pipeline {
stage('Lint and Test') {
environment {
NODE_ENV = "development"
NODE_OPTIONS="--experimental-vm-modules"
Copy link
Member

Choose a reason for hiding this comment

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

do we still need this for 18? 18 should have full module support
if not, do you want to goto 20 instead? I don't really like using the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is only used during lint+test stage. I think it's more related to Jest implementation than Node version, see https://jestjs.io/docs/ecmascript-modules

@halkeye
Copy link
Member

halkeye commented Jun 28, 2023

Also Eslint says p-queue is not reachable even though it clearly is.

can you tell me how to generate that output?

@zbynek
Copy link
Contributor Author

zbynek commented Jun 30, 2023

@halkeye see https://github.com/jenkins-infra/plugin-site/pull/1429/checks?check_run_id=14690302296 -- caused by reverting Eslint config in 618e48d
Apparently that's just a bug in Eslint import-js/eslint-plugin-import#1810

@halkeye
Copy link
Member

halkeye commented Jul 7, 2023

I'm pretty checked out. If you think its a win, go ahead and merge it. I'm still a little uncomfortable with --experimental-vm-modules but I know how messy jest can get.

@zbynek zbynek merged commit 7b331a4 into jenkins-infra:master Jul 7, 2023
4 checks passed
@zbynek zbynek deleted the esm branch July 7, 2023 05:48
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

Successfully merging this pull request may close these issues.

3 participants