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

Run Bundlemon on release optimized build #7934

Merged
merged 1 commit into from Jan 21, 2022
Merged

Run Bundlemon on release optimized build #7934

merged 1 commit into from Jan 21, 2022

Conversation

jonkoops
Copy link
Collaborator

@jonkoops jonkoops commented Jan 20, 2022

This should resolve the issues we found under #7905, namely the size check showing up twice. Also runs Bundlemon against a release optimized build to ensure that the file size is reported accurately.

@jonkoops jonkoops added the workflow Anything related to Leaflet development process and tools label Jan 20, 2022
@jonkoops jonkoops requested a review from mourner January 20, 2022 17:15
@jonkoops jonkoops self-assigned this Jan 20, 2022
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Nice! Although I don't fully understand why we need a separate workflow to compare production builds — can you explain? I thought removing duplication is fixed solely by the config.

Also, nit but I wonder why we see those small +/- ~30 bytes changes in the check when the bundle itself shouldn't change.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Per author's comment, let's keep it in the main workflow — it seems separating it is not necessary after all.

@jonkoops
Copy link
Collaborator Author

Also, nit but I wonder why we see those small +/- ~30 bytes changes in the check when the bundle itself shouldn't change.

This is because we are currently running a development build on the main branch. This build will always add GIT information such as the branch and short ref (see here). This is the primary reason why I've put the code in a separate workflow.

I think I could make it so that the main workflow also does a production build, let me experiment a little.

@mourner
Copy link
Member

mourner commented Jan 21, 2022

This build will always add GIT information such as the branch and short ref (see here).

Ah, I got confused by the terminology — it's still a production build (it's minified, etc.), just not a release build. Hashes are fine I think, after all both branch and main builds should have them. We could remove branch name from the version name to keep them the same length, but also not a big deal if there are small differences shown by the checks.

@Falke-Design
Copy link
Member

I see the project is built by the workflow, is it possible to get access to the dist folder? To create demos with the new leaflet-src?

@jonkoops jonkoops changed the title Split out Bundlemon into seperate workflow Run Bundlemon on release optimized build Jan 21, 2022
@jonkoops
Copy link
Collaborator Author

@mourner My bad, yeah I meant the release optimized build in this case, which does not include any GIT information.

I split the build out into two distinct steps now, one for a release build and one for a regular build. Bundlemon will now always compare the file size of the release build (both on main as the PR) so that no file size changes can sneak in from the build system.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@jonkoops jonkoops merged commit fe034ac into Leaflet:main Jan 21, 2022
@jonkoops jonkoops deleted the seperate-bundlemon-workflow branch January 21, 2022 12:11
JLyne pushed a commit to JLyne/Leaflet that referenced this pull request Jan 21, 2022
JLyne pushed a commit to JLyne/Leaflet that referenced this pull request Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow Anything related to Leaflet development process and tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants