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

doc: add initial doc on how to update cjs-module-lexer #43255

Closed
wants to merge 21 commits into from

Conversation

mhdawson
Copy link
Member

Add some initial doc based on discussion with Guy
Bedford.

Signed-off-by: Michael Dawson [email protected]

Add some initial doc based on discussion with Guy
Bedford.

Signed-off-by: Michael Dawson <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 30, 2022
@mhdawson
Copy link
Member Author

@guybedford I think this reflects what I understood from our discusison last week. Could you take a look to confirm?

@richardlau
Copy link
Member

There's a link in the ESM docs that is supposed to be updated to match the version of the lexer being used:

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

Signed-off-by: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member Author

@richardlau thanks, added that as a step.

doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Instead of documentation, I wonder if it wouldn't be more worth to add an automation.

doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
doc/contributing/maintaining-cjs-module-lexer.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member Author

mhdawson commented Jun 1, 2022

Instead of documentation, I wonder if it wouldn't be more worth to add an automation.

@aduh95, I think automation is always good, but even if that existed, some doc that points to the automation which could then be the doocument for the specific steps still makes sense. I know I ran across some scripts in the repo that help to build some of the other dependencies but only ran across them by accident. Long way of saying that I think we need an entry point doc for each dependency to make other parts discoverable. If we had automation for everything and there was nothing else to say/explain then we might be able to consolidate into fewer docs.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this Michael.

As mentioned, at some point it may be worthwhile to run a recent Wasm build with fixed versions of the toolchain, note those toolchain versions and update the Wasm binary.

The project is very much in a maintenance mode and will only be getting bug fixes and not feature development going forward.

@mhdawson
Copy link
Member Author

@aduh95 accepted all of your suggestions

mhdawson added a commit that referenced this pull request Jun 14, 2022
Add some initial doc based on discussion with Guy
Bedford.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #43255
Reviewed-By: Guy Bedford <[email protected]>
@mhdawson
Copy link
Member Author

Landed in cc82f6f

@mhdawson mhdawson closed this Jun 14, 2022
danielleadams pushed a commit that referenced this pull request Jun 16, 2022
Add some initial doc based on discussion with Guy
Bedford.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #43255
Reviewed-By: Guy Bedford <[email protected]>
@danielleadams danielleadams mentioned this pull request Jun 16, 2022
targos pushed a commit that referenced this pull request Jul 12, 2022
Add some initial doc based on discussion with Guy
Bedford.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #43255
Reviewed-By: Guy Bedford <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Add some initial doc based on discussion with Guy
Bedford.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #43255
Reviewed-By: Guy Bedford <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Add some initial doc based on discussion with Guy
Bedford.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs/node#43255
Reviewed-By: Guy Bedford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants