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.

Sign up for GitHub

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

Jump to bottom

tools: fix highlight.js and without-intl compatibility #41086

Closed
wants to merge 2 commits into from
Closed

tools: fix highlight.js and without-intl compatibility #41086

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 4, 2021

Fixes: #41077

@Trott Trott requested review from richardlau and targos December 4, 2021 17:04
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2021
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Dec 4, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott Trott added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Dec 4, 2021
@Trott Trott force-pushed the fix-highlight-icu branch from c45968b to 844aaf4 Compare December 4, 2021 18:14
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member Author

Trott commented Dec 4, 2021

OK, this is now fixed and running in CI.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@richardlau
Copy link
Member

The message from the first commit should be reworded -- we were not loading/registering the languages until this PR.

Trott added 2 commits December 4, 2021 12:04
@Trott
tools: import only highlight.js core
8895e23 
Importing everything from highlight.js won't work in without-intl
builds. Import only core and register only the languages we need.

Fixes: nodejs#41077
@Trott
tools: update highlight.js and other doc deps
4613506
@Trott Trott force-pushed the fix-highlight-icu branch from 844aaf4 to 4613506 Compare December 4, 2021 20:05
@Trott
Copy link
Member Author

Trott commented Dec 4, 2021 •
edited
Loading

The message from the first commit should be reworded -- we were not loading/registering the languages until this PR.

Thanks. I reworded it.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 4, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions bot mentioned this pull request Dec 5, 2021
Open
20 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41344/

@targos
Copy link
Member

targos commented Dec 5, 2021

I won't block this because the work was already done, but in my opinion, we are not supposed to make every supported build option work with everything. Building the Node.js docs is certainly not a use case for the build without Intl.

@Trott Trott mentioned this pull request Dec 5, 2021
Merged
@Trott
Copy link
Member Author

Trott commented Dec 5, 2021

I won't block this because the work was already done, but in my opinion, we are not supposed to make every supported build option work with everything. Building the Node.js docs is certainly not a use case for the build without Intl.

You might prefer #41091.

@github-actions github-actions bot mentioned this pull request Dec 6, 2021
Open
21 tasks
@Trott
Copy link
Member Author

Trott commented Dec 6, 2021

This is theoretically ready to land but I suspect #41091 is the preferred solution, so I'll put this in draft mode for now. I'll close it if #41091 lands. If #41091 stalls for some reason, I'll put this back to "ready for review".

@Trott Trott marked this pull request as draft December 6, 2021 21:31
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 6, 2021
@github-actions github-actions bot mentioned this pull request Dec 7, 2021
Open
46 tasks
@Trott
Copy link
Member Author

Trott commented Dec 7, 2021

#41091 landed instead, so closing this.

@Trott Trott closed this Dec 7, 2021
This was referenced Dec 8, 2021
Open
Open
Open
@Trott Trott deleted the fix-highlight-icu branch September 25, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@mscdex mscdex mscdex left review comments

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@lpinca lpinca lpinca approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

doc build problem in withoutintl builds
7 participants
@Trott @nodejs-github-bot @richardlau @targos @mscdex @lpinca @VoltrexKeyva