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 using JSON modules loading of package.json #797

Conversation

camcam-lemon
Copy link
Contributor

📑 Summary

The loading of package.json has been updated to JSON modules.
Additionally, the setup of Babel has been configured to allow standard and jest to interpret JSON modules.

Resolves #796

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 🔖 targeted master branch

Co-authored-by: camcam-lemon <omega.camcamlemon@gmail.com>
@camcam-lemon camcam-lemon changed the title Update using JSON modules for package.json Update using JSON modules loading of package.json Nov 29, 2024
@@ -39,6 +39,9 @@
"puppeteer": "^23"
},
"devDependencies": {
"@babel/core": "^7.26.0",
"@babel/eslint-parser": "^7.25.9",
"@babel/plugin-syntax-import-attributes": "^7.26.0",
Copy link
Contributor Author

@camcam-lemon camcam-lemon Nov 29, 2024

Choose a reason for hiding this comment

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

Babel is required to run the following

  • standard
    • @babel/core
    • @babel/eslint-parser
  • jest
    • @babel/plugin-syntax-import-attributes

@camcam-lemon
Copy link
Contributor Author

The test is failing・・・
It passed locally, so I'm not sure why.
I'll investigate later.

@@ -5,11 +5,8 @@ import { resolve } from 'import-meta-resolve'
import path from 'path'
import puppeteer from 'puppeteer'
import url from 'url'
import pkg from '../package.json' with { type: 'json' }
Copy link
Member

Choose a reason for hiding this comment

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

Oh no! I'm really sorry, but it looks like @mermaid-js/mermaid-cli supports Node.JS v18.19, but only Node.JS v18.20 added support for with { type: 'json' }. Previously, it was assert { type: 'json' }!

See https://nodejs.org/en/blog/release/v18.20.0

Unfortunately, since dropping support for Node.JS v18.19 would be a breaking change, we won't be able to merge this PR until we are ready to release @mermaid-js/mermaid-cli v12, which might be a while!


Maybe as an alternative to fix #796, could we instead just avoid loading package.json?

Maybe we can make a new file like src/version.js that just consists of:

export const version = '11.4.0';

And we can make a npm version script that just automatically creates this file and adds it to git using git add, to make sure it's kept in sync with the package.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh...
No, I should be the one apologizing.
I should have verified more thoroughly about the Node.js feature release.

Maybe as an alternative to fix #796, could we instead just avoid loading package.json?
Maybe we can make a new file like src/version.js that just consists of:

That's cool!!
You're absolutely right!
I agree with that idea as well!
This task will discard all the current PR's changes, so I will create a separate PR for this work!

When releasing, the package version is referenced as env.RELEASE_VERSION within the github actions, so I plan to try using this variable.
It seems that it can also be referenced using package.json vars, but I believe it's better to refer to the value from the github actions.
I think that accessing it from two different contexts could potentially lead to bugs in the future.

See: https://github.com/mermaid-js/mermaid-cli/blob/master/.github/workflows/release-publish.yml
See: https://docs.npmjs.com/cli/v9/using-npm/scripts#packagejson-vars

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 replaced PR #798 .
Once you confirm my comment, I will close this PR.

Copy link
Member

Choose a reason for hiding this comment

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

🚀

I should have verified more thoroughly about the Node.js feature release.

Nah, it's my fault too! I even looked at the https://nodejs.org/docs/latest-v18.x/api/esm.html#import-specifiers page and it seemed fine! But I should have also looked at the https://nodejs.org/docs/v18.19.0/api/esm.html#import-specifiers page too 😅

This task will discard all the current PR's changes, so I will create a separate PR for this work!

Sounds perfect to me! Your PR #798 looks great to me!

I have spotted a bug in it, but I've got a potential fix in #798 (comment) that should solve it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for everything!
I also checked comments.
Let's close this PR and continue in #798 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request:Update package.json to Use import' with {type: 'json'} Instead of require
2 participants