Skip to content

ref(build): Use sucrase for watch npm builds #5059

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

Merged
merged 2 commits into from
May 11, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented May 10, 2022

This follows up on #5035, which switched our main build commands to use rollup and sucrase instead of tsc, and brings that same change to our build:watch commands.

Note: Running either of these watch commands produces different feedback than the tsc watch commands we're used to, in that the build:types component auto-watches dependencies (and will trigger a cascading rebuild on change), but the build:rollup component doesn't (and therefore only rebuilds the changed package). The reason for this is that sucrase and rollup are truly only transpiling (IOW, smushing the code touching other packages around in a non-semantic way, only really worried about whether it's legal JS, not whether it plays the way it should with those other packages), not compiling (actually understanding the entire, cross-package AST and refusing to produce code if the interfaces between packages don't match up). The utility of tsc's rebuild of dependent packages was never actually to change the built code of those packages, it was just that the rebuild attempt forced a cascading typecheck, so we could know what we had to change in the dependent packages' code. The process building types still serves this function, but the process building code doesn't need to and therefore there's no need for the cascade. (As you'll see in the conversation below, I myself was fuzzy on this point at first, which is why I'm spelling it out here quite so explicitly, in case any future readers stumble into the same wrong assumptions I did.)

@github-actions
Copy link
Contributor

github-actions bot commented May 10, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 18.78 KB (-6.78% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 58.38 KB (-9.65% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.54 KB (-7% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 52.61 KB (-9.26% 🔽)
@sentry/browser - Webpack (gzipped + minified) 19.33 KB (-16.83% 🔽)
@sentry/browser - Webpack (minified) 61.47 KB (-24.77% 🔽)
@sentry/react - Webpack (gzipped + minified) 19.35 KB (-16.87% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 42.81 KB (-10.91% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 24.43 KB (-6.31% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 23 KB (-6.06% 🔽)

@lobsterkatie lobsterkatie force-pushed the kmclb-use-sucrase-for-watch-npm-builds branch 2 times, most recently from 30c2d1c to 6d7f34d Compare May 11, 2022 04:08
@lobsterkatie lobsterkatie changed the title ref(build): Use sucrase for watch npm builds ref(build): Use sucrase for top-level watch npm builds May 11, 2022
@lobsterkatie lobsterkatie marked this pull request as ready for review May 11, 2022 04:31
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

I don't understand why packages need to watch their dependants. In cjs/esm, packages should be able to be built independently from each other, meaning for example, a change in core will not influence the generated esm/cjs code in browser. Am I understanding this right? This feels redundant to me.

@lobsterkatie
Copy link
Member Author

a change in core will not influence the generated esm/cjs code in browser

It won't change it, but it may force changes to it. Say browser uses a function from core, and I change the signature in core. If browser is not watching core for changes, it won't get rebuilt, and therefore won't error because now it's using the function the wrong way.

@lobsterkatie lobsterkatie force-pushed the kmclb-use-sucrase-for-watch-npm-builds branch from 6d7f34d to a9507b1 Compare May 11, 2022 13:48
@lforst
Copy link
Member

lforst commented May 11, 2022

now it's using the function the wrong way

Misusing imported functions shouldn't affect the building of a package. It affects the typing - we should definitely watch dependencies when running build:types:watch - but building of esm/cjs modules should be completely isolated. Or am I misunderstanding something?

@lobsterkatie
Copy link
Member Author

Misusing imported functions shouldn't affect the building of a package. It affects the typing - we should definitely watch dependencies when running build:types:watch - but building of esm/cjs modules should be completely isolated. Or am I misunderstanding something?

No, upon further thought, I think I may be the one who's conflating things. Compiling and typechecking have been so intertwined in my head for so long that I don't think I stopped to think just how much this change to sucrase divorces the two. I'll pull that part out (which makes things much easier, BTW!) and stash it in a branch in case this logic somehow doesn't hold (though I think you're right, I don't think we'll need it).

Thanks for pushing back on this!

@lobsterkatie lobsterkatie force-pushed the kmclb-use-sucrase-for-watch-npm-builds branch from a9507b1 to b59a360 Compare May 11, 2022 17:30
@lobsterkatie lobsterkatie changed the title ref(build): Use sucrase for top-level watch npm builds ref(build): Use sucrase for watch npm builds May 11, 2022
@lobsterkatie lobsterkatie merged commit 7723d32 into 7.x May 11, 2022
@lobsterkatie lobsterkatie deleted the kmclb-use-sucrase-for-watch-npm-builds branch May 11, 2022 18:31
@AbhiPrasad AbhiPrasad added this to the 7.0.0 milestone May 12, 2022
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
This follows up on #5035, which switched our main build commands to use rollup and sucrase instead of tsc, and brings that same change to our `build:watch` commands.

Note: Running either of these `watch` commands produces different feedback than the `tsc` watch commands we're used to, in that the `build:types` component auto-watches dependencies (and will trigger a cascading rebuild on change), but the `build:rollup` component doesn't (and therefore only rebuilds the changed package). The reason for this is that sucrase and rollup are truly only *trans*piling (IOW, smushing the code touching other packages around in a non-semantic way, only really worried about whether it's legal JS, not whether it plays the way it should with those other packages), not *com*piling (actually understanding the entire, cross-package AST and refusing to produce code if the interfaces between packages don't match up). The utility of `tsc`'s rebuild of dependent packages was never actually to change the built code of those packages, it was just that the rebuild attempt forced a cascading typecheck, so we could know what _we_ had to change in the dependent packages' code. The process building types still serves this function, but the process building code doesn't need to and therefore there's no need for the cascade. (As you'll see in the conversation below, I myself was fuzzy on this point at first, which is why I'm spelling it out here quite so explicitly, in case any future readers stumble into the same wrong assumptions I did.)
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.

3 participants