Skip to content

fix(build) :: import 3rd party frontend assets from a list - #1456

Merged
lovasoa merged 1 commit into
mainfrom
81reap/npm-1
Sep 17, 2026
Merged

lovasoa merged 1 commit into
mainfrom
81reap/npm-1

Conversation

@81reap

@81reap 81reap commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Before assets were build by scanning for /* !include https://... */ comments to fetch the asset from a CDN.

Now the assets are a list in build.rs.

This ends up fixing another bug where DOC_RS would return in the middle of a scan on the first include which would leave the gzip stream unfinished.


Stack created with GitHub Stacks CLIGive Feedback 💬

@81reap
81reap added this pull request to stack #1462 September 16, 2026 21:27
@81reap 81reap linked an issue Sep 16, 2026 that may be closed by this pull request
@81reap
81reap requested a review from lovasoa September 16, 2026 21:41

@lovasoa lovasoa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi! Can you please organize the PRs so that all are small enough to be reviewed, yet none breaks the project?

@81reap

81reap commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

@lovasoa not sure how to do both here. I chose to split up this and #1457 as the diff was getting quite big.

That said, with GitHub stacks, we can review both PRs and merge them together so that project is never in a broken state!

@lovasoa

lovasoa commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

It looks like the next pr contains changes unrelated to what you are doing here.

I think you could make a good no-brainer pr with net negative lines of code by keeping the exact exact same output files, but removing the include comment mechanism, hardcoding the concatenation logic in build.rs and copying from node modules instead of the web.

Before assets were build by scanning for `/* !include https://... */` comments to fetch the asset from a CDN.

Now the assets are a list in `build.rs`.

This ends up fixing another bug where `DOC_RS` would return in the middle of a scan on the first include which would leave the gzip stream unfinished.
@81reap

81reap commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

I originally chose not to do it that way because it required refactoring build.rs in almost every commit in the stack and temporarily importing node_modules into the docker image. I didn't think that would be helpful to the review process.

Pushed up a new stack tho.

@81reap 81reap changed the title fix(frontend) :: take browser dependencies from npm fix(build) :: import 3rd party frontend assets from a list Sep 17, 2026

@lovasoa lovasoa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks 👍

@lovasoa
lovasoa merged commit c231815 into main Sep 17, 2026
51 checks passed
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.

Split the build

2 participants