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

feat: use ES modules #1007

Merged
merged 5 commits into from
Dec 10, 2024
Merged

feat: use ES modules #1007

merged 5 commits into from
Dec 10, 2024

Conversation

JasonBerry
Copy link
Contributor

@JasonBerry JasonBerry commented Dec 9, 2024

  • removed the reliance on the esm library which has been dead for some time
  • updated to Node 18, which is the LTS version still being maintained - Node 12 was EOL'd 2022-04-30
  • tried to modify as little as possible to keep the PR small

Fixes #1006

BREAKING CHANGE: code is in in ES Modules now and earliest node support is 18

@Haroenv Haroenv self-requested a review December 9, 2024 12:31
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

checked that all files are correctly imported with seeing no matches for from '\./.*[^js]';, and runs correctly. thanks!

@@ -1,10 +1,10 @@
#!/usr/bin/env node

require = require('esm')(module);
(async function() {
(async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about the history but this likely was an async iife to ensure all errors are caught. Not sure that's needed really tbh. Can be changed in a separate PR and release to be just a regular sync module maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm easy with however you want to do it. I was just trying to keep changes to a minimum!

.mockImplementation(
({ formatPullRequestTitle, nextVersion, releaseType }) => {
return [
formatPullRequestTitle({
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change by the way? was the formatting not needed in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if/how this was ever working!

In createPullRequest.js formatPullRequestTitle is never passed to formatPullRequestMessage and so formatPullRequestTitle is undefined and not able to be called. The relevant bit is:

    const title = formatPullRequestTitle({ version: nextVersion, releaseType });
    const message = formatPullRequestMessage({
      repo,
      repoURL,
      baseBranch,
      stagingBranch,
      destinationBranch: baseBranch,
      releaseType,
      diffURL,
      currentVersion,
      nextVersion,
      publishCommands,
      title,
    });

It doesn't make any sense that it was ever working.

@Haroenv Haroenv merged commit f33b9bc into main Dec 10, 2024
1 check passed
@Haroenv Haroenv deleted the esm branch December 10, 2024 09:43
@@ -69,15 +69,8 @@ describe('createPullRequest', () => {
.mockImplementation(({ version, type }) => `# v${version} (${type})`);
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 should actually be:

.mockImplementation(({ version, releaseType }) => `# v${version} (${releaseType})`);

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.

Incompatibility with Node.js v22.x (Assertion failed in esm)
2 participants