refactor[ci/build]: preserve header format in artifacts #27671
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
scripts/rollup/build.js
Outdated
| // We were passing sources without license headers to the Closure compiler. | ||
| // The reason for it is its optimization for multiline comment: | ||
| // it will change the format by removing * prefixes This doesn't work well with Haste. | ||
| const preMinifiedBundleSource = Wrappers.wrapWithLicenseHeader( |
There was a problem hiding this comment.
❓ Hmm. I am curious if the sourcemap transforms are still going to be accurate vs the output file contents if we don't include the license contents before the sourcemap gets generated.
This is an area where my own knowledge is very hazy. My coworker @hbenl might have more knowledge here.
My concern is that sending one version of the source file through Closure, and writing a modified file to disk, might cause line numbers to get mismatched when it's used in a debugger (such that it thinks you're paused on line 37 of a file, but that turns out to be a } character or something instead of the actual meaningful line of code).
I'd recommend doing some checks with the artifacts from this PR to see if setting breakpoints and stepping still seems to result in correct lines and variables being handled.
There was a problem hiding this comment.
You are right, mappings will also need to be updated. I will look for other approach here.
There was a problem hiding this comment.
I've ended up with manually updating the header to follow certain format. I couldn't find any annotations for Closure compiler that would allow us to preserve original header-comment format, unfortunately.
I've validated this in my template React-app, here is the demo, where I've replaced react-dom package with the one with sourcemaps:
Screen.Recording.2023-11-09.at.11.06.28.mov
There was a problem hiding this comment.
Alright, I think I found the best option for us:
- Artifacts with sourcemaps will get header attached before, so it will effect the sourcemaps and they will be generated correctly.
- Artifacts without sourcemaps (facebook-bundles are included in this category) will get header attached after Closure compiler, so they will preserve correct format of the header.
I've validated it the same way as above, sourcemaps work.
hoxyq
force-pushed
the
refactor/preserve-header-format-in-artifacts
branch
from
9afb325 to
b34fb8b
Compare
hoxyq
force-pushed
the
refactor/preserve-header-format-in-artifacts
branch
from
b34fb8b to
9708b15
Compare
hoxyq
force-pushed
the
refactor/preserve-header-format-in-artifacts
branch
from
9708b15 to
a21c5af
Compare
|
By looking at the https://react-builds.vercel.app/commits/a21c5afe9675a731b65183d3e8b0f19aa5201aa3/files/facebook-www, I can validate that production and profiling FB-bundles now preserve header style. |
kassens
left a comment
kassens
left a comment
There was a problem hiding this comment.
The complexity of this build script is a bit annoying, but that's just a side note.
Looks good to me!
In order to make Haste work with React's artifacts, It is important to
keep headers in this format:
```
/**
* ...
...
* ...
*/
```
For optimization purposes, Closure compiler will actually modify these
headers by removing * prefixes, which is expected.
We should pass sources to the compiler without license headers, with
these changes the current flow will be:
1. Apply top-level definitions. For UMD-bundles, for example, or
DEV-only bundles (e. g. `if (__DEV__) { ...`)
2. Apply licence headers for artifacts with sourcemaps: oss-production
and oss-profiling bundles, they don't need to preserve the header format
to comply with Haste. We need to apply these headers before passing
sources to Closure, so it can build correct mappings for sourcemaps.
3. Pass these sources to closure compiler for minification and
sourcemaps building.
4. Apply licence headers for artifacts without sourcemaps: dev bundles,
fb bundles. This way the header style will be preserved and not changed
by Closure.
DiffTrain build for [c47c306](c47c306)
Updates React from 746890329 to 0e352ea01. - facebook/react#27684 - facebook/react#27665 - facebook/react#27671 - facebook/react#27680
In order to make Haste work with React's artifacts, It is important to
keep headers in this format:
```
/**
* ...
...
* ...
*/
```
For optimization purposes, Closure compiler will actually modify these
headers by removing * prefixes, which is expected.
We should pass sources to the compiler without license headers, with
these changes the current flow will be:
1. Apply top-level definitions. For UMD-bundles, for example, or
DEV-only bundles (e. g. `if (__DEV__) { ...`)
2. Apply licence headers for artifacts with sourcemaps: oss-production
and oss-profiling bundles, they don't need to preserve the header format
to comply with Haste. We need to apply these headers before passing
sources to Closure, so it can build correct mappings for sourcemaps.
3. Pass these sources to closure compiler for minification and
sourcemaps building.
4. Apply licence headers for artifacts without sourcemaps: dev bundles,
fb bundles. This way the header style will be preserved and not changed
by Closure.
In order to make Haste work with React's artifacts, It is important to
keep headers in this format:
```
/**
* ...
...
* ...
*/
```
For optimization purposes, Closure compiler will actually modify these
headers by removing * prefixes, which is expected.
We should pass sources to the compiler without license headers, with
these changes the current flow will be:
1. Apply top-level definitions. For UMD-bundles, for example, or
DEV-only bundles (e. g. `if (__DEV__) { ...`)
2. Apply licence headers for artifacts with sourcemaps: oss-production
and oss-profiling bundles, they don't need to preserve the header format
to comply with Haste. We need to apply these headers before passing
sources to Closure, so it can build correct mappings for sourcemaps.
3. Pass these sources to closure compiler for minification and
sourcemaps building.
4. Apply licence headers for artifacts without sourcemaps: dev bundles,
fb bundles. This way the header style will be preserved and not changed
by Closure.
DiffTrain build for commit c47c306.
5 participants
In order to make Haste work with React's artifacts, It is important to keep headers in this format:
For optimization purposes, Closure compiler will actually modify these headers by removing * prefixes, which is expected.
We should pass sources to the compiler without license headers, with these changes the current flow will be:
if (__DEV__) { ...)