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

deps: update cjs-module-lexer to 2.0.0 #56855

Merged
merged 2 commits into from
Feb 4, 2025

Conversation

mhdawson
Copy link
Member

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 31, 2025
@mhdawson
Copy link
Member Author

This should be landed in preference to the autogenerated PR as in addition to the update it:

  1. updates the updater script so that we can rebuild wasm and generated files from what is in the deps directory
  2. it removes/adjust files to be consistent with what the updated version of the updater scripts uses/needs
  3. it updates a test that needed to be adjusted for 1

It was generated by adjusting the files as needed, and then running the update, followed by updating the test that I discovered needed adjustment.

@mhdawson
Copy link
Member Author

@marco-ippolito FYI

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson mhdawson force-pushed the cjs-module-lexer-updater branch from 24da6e7 to a0e6994 Compare January 31, 2025 17:41
@mhdawson mhdawson changed the title deps: update cjs-module-lexer to 1.4.2 deps: update cjs-module-lexer to 2.0.0 Jan 31, 2025
@marco-ippolito marco-ippolito self-requested a review January 31, 2025 20:14
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

RSLGTM

Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

LGTM, off topic, since cjs-module-lexer is written in C we could build it directly without requiring a wasm?

@richardlau
Copy link
Member

Documentation needs to be updated.

* Update the link to the cjs-module-lexer in the list at the end of
[doc/api/esm.md](../../api/esm.md)
to point to the updated version.

<!-- Note: The cjs-module-lexer link should be kept in-sync with the deps version -->

[cjs-module-lexer]: https://github.com/nodejs/cjs-module-lexer/tree/1.2.2

Ideally the updater should take care of this.

@joyeecheung
Copy link
Member

LGTM, off topic, since cjs-module-lexer is written in C we could build it directly without requiring a wasm?

I think that would need some actual binding code to be written (or FFI support ;)) because right now it just manually exports selected functions through wasm and let the JS side drive them. It might be a fair bit faster if it can just work with buffers and we write some V8 fast APIs to work with the buffer, and refactor the ESM loader to simply pass UTF8 data in buffers around.

Signed-off-by: Michael Dawson <midawson@redhat.com>
@mhdawson
Copy link
Member Author

mhdawson commented Feb 3, 2025

@richardlau updated the doc, can look at improving updater in a follow on PR.

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.18%. Comparing base (304bb9c) to head (cb3721d).
Report is 52 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56855      +/-   ##
==========================================
- Coverage   89.20%   89.18%   -0.02%     
==========================================
  Files         663      665       +2     
  Lines      192012   192529     +517     
  Branches    36929    37036     +107     
==========================================
+ Hits       171286   171709     +423     
- Misses      13582    13637      +55     
- Partials     7144     7183      +39     

see 60 files with indirect coverage changes

@richardlau richardlau added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 4, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 4, 2025
@nodejs-github-bot nodejs-github-bot merged commit dd92abc into nodejs:main Feb 4, 2025
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in dd92abc

targos pushed a commit that referenced this pull request Feb 5, 2025
Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #56855
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants