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

Convert to ESLint 9 flat config and move to pnpm, use .mjs and new jest types #2548

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

SleeplessByte
Copy link
Member

@SleeplessByte SleeplessByte commented Oct 18, 2024

Related: exercism/javascript-test-runner#852

Similar to: exercism/typescript#1506

  • Migrate to pnpm: means less space used, and installing can often be done without internet connection as long as you download the exercise first. This is something we discussed in the past (not specifically pnpm, but "how can we share dependencies", and this I think is a good way to do it, for now. If corepack gets removed in the future, our migration path is as "simple" as removing corepack from the calls. Everything keeps working.
  • Upgrade to ESLint 9: This is a significant upgrade that uses a new config flat file format. We already have it on TypeScript, so I think we should be good.
  • Upgrade SDKs: for VSCode integration when working with this repository.
  • Sync
  • Format files
  • Fix .vscode files, upgrade all tests to new format
  • Update workflows to Node LTS @ 20
  • Fix lint issues
  • Remove babel transpilation for common actions: We really don't need it anymore. Check https://node.green/#ES2022!
  • Simplify changed files check: The new method is much faster and doesn't rely on the first page of items in the change dataset.

scripts/directory-check.mjs Dismissed Show dismissed Hide dismissed
@SleeplessByte SleeplessByte added the x:size/large Large amount of work label Oct 18, 2024
@SleeplessByte SleeplessByte added hacktoberfest-accepted Opt-in to hacktoberfest hacktoberfest Hacktoberfest issues! Everyone allowed <3 labels Oct 18, 2024
@SleeplessByte
Copy link
Member Author

SleeplessByte commented Nov 1, 2024

Okay. So.

This PR works and relies on exercism/javascript-test-runner#852, but I do not know if that PR will work as it requires the tmp directory to be mounted with exec on as that's where pnpm installs the executables.

My plan is:

  1. test the test runner against a non-upgraded exercise solution (so one that expects npm instead of pnpm)
  2. merge the test runner
  3. try this online

If that works, mark this as ready and we are good to go! If it doesn't work, I will revert the test runner and message Jeremy.

@SleeplessByte SleeplessByte marked this pull request as ready for review November 1, 2024 12:44
@Cool-Katt
Copy link
Contributor

What a monster PR! Not even sure where to start with this...

@SleeplessByte
Copy link
Member Author

Isn't it fun 📦.

Basically: eslint 9, pnpm instead of npm. Check that the instructions to install are clear and that you don't absolutely hate it.

@Cool-Katt
Copy link
Contributor

Looks good from what I could see, appart from a few leftover comments.

@SleeplessByte
Copy link
Member Author

@Cool-Katt where's the comments?

* Run this script (from root directory): npx babel-node scripts/sync
* Run this script (from root directory):
*
* $ corepack yarn sync
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be pnpm here instead of yarn?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes!

#!/usr/bin/env node

/**
* Run this script (from root directory): npx babel-node scripts/checksum
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover comment

@@ -7,5 +7,6 @@
// because of how whitespace is (not) rendered.
65
]
}
},
"cSpell.words": ["xtest"]
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intended for here or was it an artifact?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh. Yeah i run spellcheck locally but didn't mean to commit this I think.

@Cool-Katt
Copy link
Contributor

@SleeplessByte sorry, I accidentally forgot to post the actual review. Lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Hacktoberfest issues! Everyone allowed <3 hacktoberfest-accepted Opt-in to hacktoberfest x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants