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

refactor[ci/build]: support FB_WWW_BROWSER_SCRIPT bundle type #27664

Closed

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Nov 8, 2023

Follow-up on #26446.

Fixes 2 issues:

  1. We are manually copying unstable_server-external-runtime.js into facebook-www when committing artifacts to Meta's monorepo.
    • This diverges from the local flow with just running yarn build: you will not get build/facebook-www/unstable_server-external-runtime.js in this case.
    • Because build/oss-stable/react-dom/unstable_server-external-runtime.js is actually included into react-dom npm package, we ship sourcemaps for it. We don't need sourcemaps for Meta's versions of these artifacts, though, it should be built separately with a different bundle type.
  2. unstable_server-external-runtime.js doesn't have any effect on the hash that is created here https://github.com/facebook/react/blob/main/scripts/rollup/build-all-release-channels.js#L165-L173 or here
    https://github.com/facebook/react/blob/main/scripts/rollup/build-all-release-channels.js#L239-L247. I can double-check this, but it probably means that if only sources of this artifact change, it won't create a new react version identifier, which is based on hash.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Nov 8, 2023
Comment on lines +172 to +174
if (fileName !== 'unstable_server-external-runtime.js') {
fs.renameSync(filePath, filePath.replace('.js', '.classic.js'));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have access to bundle types at this point. Instead of relying on fileName I can parse bundles array and try finding everything what has bundleType FB_WWW_BROWSER_SCRIPT, if that seems like a better option

@hoxyq
Copy link
Contributor Author

hoxyq commented Nov 8, 2023

This would require more changes, than I've expected. Since we have only 1 artifact with BROWSER_SCIRPT bundle type, we can stop generating sourcemaps for it for now. The changes in this PR will make these build scripts even more complex.

Closing this in favour of #27665.

@hoxyq hoxyq closed this Nov 8, 2023
hoxyq added a commit that referenced this pull request Nov 9, 2023
…es (#27665)

Instead of #27664, we can just
exclude `unstable_server-external-runtime.js` from having sourcemaps for
now.

We should consider removing manual copying of this artifact in
https://github.com/facebook/react/blob/52d542ad6d410008c495084f511247f43387055f/.github/workflows/commit_artifacts.yml#L136-L138


As described in #27664, this
artifact doesn't have any effect on the `hash`, which is used for
generating React version identifier.
github-actions bot pushed a commit that referenced this pull request Nov 9, 2023
…es (#27665)

Instead of #27664, we can just
exclude `unstable_server-external-runtime.js` from having sourcemaps for
now.

We should consider removing manual copying of this artifact in
https://github.com/facebook/react/blob/52d542ad6d410008c495084f511247f43387055f/.github/workflows/commit_artifacts.yml#L136-L138

As described in #27664, this
artifact doesn't have any effect on the `hash`, which is used for
generating React version identifier.

DiffTrain build for [78c71bc](78c71bc)
jerrydev0927 added a commit to jerrydev0927/react that referenced this pull request Jan 5, 2024
…es (#27665)

Instead of facebook/react#27664, we can just
exclude `unstable_server-external-runtime.js` from having sourcemaps for
now.

We should consider removing manual copying of this artifact in
https://github.com/facebook/react/blob/52d542ad6d410008c495084f511247f43387055f/.github/workflows/commit_artifacts.yml#L136-L138

As described in facebook/react#27664, this
artifact doesn't have any effect on the `hash`, which is used for
generating React version identifier.

DiffTrain build for [78c71bc545bf5c0fdeedc023b69fafe05d988067](facebook/react@78c71bc)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…es (facebook#27665)

Instead of facebook#27664, we can just
exclude `unstable_server-external-runtime.js` from having sourcemaps for
now.

We should consider removing manual copying of this artifact in
https://github.com/facebook/react/blob/52d542ad6d410008c495084f511247f43387055f/.github/workflows/commit_artifacts.yml#L136-L138


As described in facebook#27664, this
artifact doesn't have any effect on the `hash`, which is used for
generating React version identifier.
@hoxyq hoxyq deleted the build/add-fb-www-browser-script-bundle-type branch August 9, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants