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

fix: mac arm build #3697

Merged
merged 2 commits into from Aug 30, 2022
Merged

Conversation

gengjiawen
Copy link
Contributor

This internally bump brotli-size, the old version contains cpp module which not compatible on m1 mac.

The d.ts looks auto updated with microbundler update. Not sure should i revert it or leave it be.

@coveralls
Copy link

coveralls commented Aug 28, 2022

Coverage Status

Coverage increased (+0.2%) to 99.522% when pulling 03a66bc on gengjiawen:fix/mac_arm into 4672611 on preactjs:master.

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

This seems to change a bit more than just the brotli-size version, would it be worth just bumping brotli in our package json?

@rschristian
Copy link
Member

Newer microbundle versions will generate .d.ts files if "types" is specified in the package file. Will need to run it with --no-generateTypes to ensure hand-written types aren't overwritten if brotli for some reason can't be bumped on its own.

@developit
Copy link
Member

Good point @rschristian - @gengjiawen can you re-do this PR adding --no-generateTypes to each microbundle script? The result should (hopefully) be a PR that only modifies package.json and package-lock.json.

@gengjiawen
Copy link
Contributor Author

Good point @rschristian - @gengjiawen can you re-do this PR adding --no-generateTypes to each microbundle script? The result should (hopefully) be a PR that only modifies package.json and package-lock.json.

done.

@rschristian
Copy link
Member

rschristian commented Aug 30, 2022

Has the build size increased? We (unfortunately) don't seem to run compressed size action if the source hasn't been touched.

Last time I checked, updating Microbundle increased the output size by a non-negligible amount.

Edit: Microbundle can now also output .mjs. We may not need the node-13-exports script anymore, but built files to exports will certainly need a look over. On mobile at the moment, but will try to take a peek later.

@gengjiawen
Copy link
Contributor Author

Size not changed much
on master

-rw-r--r-- 1 gitpod gitpod  10K Aug 30 02:05 preact.js
-rw-r--r-- 1 gitpod gitpod 9.9K Aug 30 02:04 preact.min.js
-rw-r--r-- 1 gitpod gitpod 9.9K Aug 30 02:04 preact.mjs
-rw-r--r-- 1 gitpod gitpod 9.9K Aug 30 02:04 preact.module.js
-rw-r--r-- 1 gitpod gitpod  11K Aug 30 02:04 preact.umd.js

on this patch

-rw-r--r-- 1 gitpod gitpod 9.9K Aug 30 01:44 preact.js
-rw-r--r-- 1 gitpod gitpod 9.9K Aug 30 01:44 preact.min.js
-rw-r--r-- 1 gitpod gitpod 9.9K Aug 30 01:44 preact.mjs
-rw-r--r-- 1 gitpod gitpod 9.9K Aug 30 01:44 preact.module.js
-rw-r--r-- 1 gitpod gitpod  11K Aug 30 01:44 preact.umd.js

@rschristian
Copy link
Member

Rounding (or truncating) to the nearest 100b doesn't exactly give the clearest view. Probably doesn't make much sense to increase output size to solve a build-time platform compat issue.

Can brotli-size be upgraded independently as Jovi originally suggested? Microbundle will likely need to be updated at some point but, as it seems there are implications for doing so and it involves more moving pieces (pkg.exports, post-build scripts, etc), I'm not sure if it's worth touching to fix this specific issue if it can be avoided.

@JoviDeCroock
Copy link
Member

The thing that scares me here is that this seems to be generating additional files 😅

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

This is great! Thank you so much for taking the time to address feedback and hanging with us 👍

@JoviDeCroock @rschristian The current iteration works fine for me and seems to generate the same output as before. I don't see any additional files being generated on my end.

@JoviDeCroock
Copy link
Member

@marvinhagemeister generates an extra .mjs file for all packages, if that isn't an issue I am okay with merging

       1381 B: hooks.mjs.gz
       1277 B: hooks.mjs.br

@marvinhagemeister
Copy link
Member

@JoviDeCroock this is actually great that microbundle generates these by default now. This means we can drop the script in ./config/node-13-export.js now. That kinda added the missing functionality of generating those .mjs files.

@JoviDeCroock
Copy link
Member

Want to do that in a follow-up or do we do that now? 😅

@marvinhagemeister
Copy link
Member

I'm fine with a follow up. I think we dragged this PR long enough.

@marvinhagemeister marvinhagemeister merged commit 89fe31e into preactjs:master Aug 30, 2022
@rschristian
Copy link
Member

The resulting .mjs files are Microbundle's modern builds, not the desired copy of .module.js. Will follow up.

@marvinhagemeister
Copy link
Member

@rschristian oh, well in that case we should probably keep our script around. Currently it copies the .module.js file and overwrites *.mjs anyway.

@rschristian
Copy link
Member

@rschristian oh, well in that case we should probably keep our script around. Currently it copies the .module.js file and overwrites *.mjs anyway.

Made a PR but elected to disable the modern output. Figured relying on the overwrite would eventually trip someone up or create issues.

@gengjiawen gengjiawen deleted the fix/mac_arm branch December 27, 2022 06:58
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.

None yet

6 participants