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

(build) allow third-party packages to override default languages #4199

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lysxia
Copy link

@Lysxia Lysxia commented Jan 22, 2025

Changes

For example, I can define my own rust.js as a third-party package in extra/, then node tools/build.js rust will pick up my version instead of the default version.

Checklist

  • Added markup tests, or they don't apply here because this just modifies the build script
  • Updated the changelog at CHANGES.md

@joshgoebel
Copy link
Member

I think this is ok, since one would only ever do this on purpose - but I'm going to give it just a little more thought.

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

1 file changed

Total change -1 B

View Changes
file base pr diff
highlight.min.js 8.23 KB 8.23 KB -1 B

@joshgoebel
Copy link
Member

joshgoebel commented Jan 24, 2025

Which build or builds are you using? The current implementation really wasn't designed to deal with overlapping names at all. I worry any behavior here may be accidental so I need to take a closer look.

I'm fine with the idea of this change though (and it likely was my original intent - that 3rd party get added last, though I'm not sure I considered of dups).

@joshgoebel
Copy link
Member

joshgoebel commented Jan 24, 2025

Ah, filter makes this work because it's scanning the list top-down to find matches (and it will find the first rust when there are many)... I think a better solution here would be to first call reverse on all languages in dependencies.js#filter so that the filter works from bottom to top - so that languages added later to the list win when there is a name match. (ie, it would feel like latter ones overwrite former, the behavior you'd see if we were using a hash, etc)

Could you give that a shot and update this PR?

Technically this would be a breaking change, but I may let it slide since this strikes me as very edge case.

@Lysxia Lysxia force-pushed the third-party-first branch from 67e407c to bd8de6c Compare January 24, 2025 16:38
@Lysxia
Copy link
Author

Lysxia commented Jan 24, 2025

Changing filter makes sense to me. How this commit?

At the moment I'm using the browser build.

@joshgoebel
Copy link
Member

Changing filter makes sense to me. How this commit?

Looks good. Safe to assume you tested and it also gets the job done?

@Lysxia
Copy link
Author

Lysxia commented Jan 25, 2025

Indeed this works for me.

Copy link
Member

@joshgoebel joshgoebel left a comment

Choose a reason for hiding this comment

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

Tested by PR submitter, looks good to me. CI green.

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

3 files changed

Total change +3 B

View Changes
file base pr diff
es/core.min.js 8.19 KB 8.19 KB +1 B
es/highlight.min.js 8.19 KB 8.19 KB +1 B
highlight.min.js 8.23 KB 8.23 KB +1 B

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.

2 participants