ref(build): Introduce root build directory in @sentry/browser

[Lms24]

This PR introduces a central build directory in @sentry/browser into which all generated JS sources (bundles, CJS, ESM) are built. I started with @sentry/browser to create a PoC for this. Once we figure it out for one package, we can adapt the changes in other ones as well , either in this PR or in subsequent PRs.

This PR:

• Adds a root build directory as the output directory for all module formats and bundles. This puts all built JS files and modules which were previously written into build (bundles). esm (es6), dist (cjs) one level behind into a central build directory. This should help keeping things more organized and more clearly arranged.

• Adds a postbuild.sh scripts which first copies all non-code assets for the NPM tarball into build and then applies modifications to package.json to e.g. adjust entry points

• Adjusts tsconfig.json further to build type declarations (*.d.ts) into a separate directory (build/types). This deduplicates the identical declarations that were peviously compiled into dist and esm.

• Introduces BUILD_DIR export in root rollup.config.js as it will (probably) be
  used in multiple packages and this lets us change the name for all of them at once.

Directory Structure

With this PR, the file structure after running yarn build is currently as follows:

browser/
├─ build/
│  ├─ bundles/
│  │  ├─ JS bundles (rollup) (es5, es6) (+min, maps)
│  ├─ dist/
│  │  ├─ CJS modules (+maps)
│  ├─ esm/
│  │  ├─ ES6 modules (+maps)
│  ├─ types/
│  │  ├─ *.d.ts files (+maps)
├─ package.json
├─ LICENSE
├─ README.md
├─ ...

NPM Package

With this PR, the NPM package tarball from yarn run pack is created from inside build. Hence, the non-code assets need to be copied to build and the entry points in package.json need to be adjusted. This happens in the postbuild.sh script which runs automatically after yarn build via the postbuild script specified in package.json. The final tarball is saved in the sentry-browser root directory.

The structure of the NPM package looks as follows:

sentry-browser-<version>.tgz/
|─ bundles/
│  ├─JS bundles (rollup) (es5, es6) (+min, maps)
├─ dist/
│  ├─ CJS modules (+maps)
├─ esm/
│  ├─ ES6 modules (+maps)
├─ types/
│  ├─ *.d.ts files (+maps)
├─ package.json
├─ LICENSE
├─ README.md

To avoid a breaking change for now, the type declarations are copied from types to dist and esm. Once v7 is released, we can remove the declarations from dist and esm. This breaking change will affect people importing from e.g. @sentry/browser/dist, instead of relying on the entry points specified in package.json. These imports will not include type declarations anymore.

ref: https://getsentry.atlassian.net/browse/WEB-650
ref: https://getsentry.atlassian.net/browse/WEB-646

[AbhiPrasad]

This will be a breaking change for the folks doing relative imports from dist or esm directly instead of relying on package.json definitions. This is not how stuff should be used - but there are def cases of it happening.

I think this is a solid change to include in the major.

[Lms24]

Yes, I was thinking about this as well. I guess we could potentially avoid the breaking change by copying all additional files that should go into the tarball to build and npm packing from build instead of the root. In this case, we could leave the entry points in package.json as they are now instead of adding build/ before them.

However, this seems to be a problem for our monorepo, as lerna seems to always take the package.json from the root of the package and for this I need to adjust the entry points. I might be missing something but my quick research on lerna config did not reveal much that could help here.

[github-actions]

size-limit report

Path	Base Size (4eadcaf)	Current Size	Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.49 KB	19.49 KB	+0.01% 🔺
@sentry/browser - ES5 CDN Bundle (minified)	62.17 KB	62.17 KB	0%
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.11 KB	18.12 KB	+0.03% 🔺
@sentry/browser - ES6 CDN Bundle (minified)	55.5 KB	55.5 KB	0%
@sentry/browser - Webpack (gzipped + minified)	22.6 KB	22.6 KB	0%
@sentry/browser - Webpack (minified)	79.21 KB	79.21 KB	0%
@sentry/react - Webpack (gzipped + minified)	22.63 KB	22.63 KB	0%
@sentry/nextjs Client - Webpack (gzipped + minified)	47.6 KB	47.6 KB	0%
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.36 KB	25.37 KB	+0.02% 🔺
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	23.72 KB	23.72 KB	+0.01% 🔺

[lobsterkatie]

This looks good so far!

It's true that this is a breaking change for people who have been using individual parts straight out of dist, and even though that's off-label, it's also true we probably shouldn't break them.

IMHO the best compromise there would be to switch to doing it this way, and then just add something like && cp -r build/dist ./dist && cp -r build/esm ./esm && cp build/types/* ./dist/ && cp build/types/* ./esm/ to the end of the build command. That way, people aren't broken, but when it comes time for the major, all we have to do is delete that bit. WDYT? (EDIT: I guess we'd have to hold off on the main/module/types changes in package.json, too, but that still leaves it a smaller change compared to doing all of this at that point.)

Separately, I think we should switch from build/dist to build/cjs, since it's more descriptive.

[lobsterkatie]

IMHO the best compromise there would be to switch to doing it this way, and then just add something like && cp -r build/dist ./dist && cp -r build/esm ./esm && cp build/types/* ./dist/ && cp build/types/* ./esm/ to the end of the build command. That way, people aren't broken, but when it comes time for the major, all we have to do is delete that bit. WDYT? (EDIT: I guess we'd have to hold off on the main/module/types changes in package.json, too, but that still leaves it a smaller change compared to doing all of this at that point.)

Transferring the results of an IRL convo here for posterity: We're going to try the strategy mentioned above, about building the npm package from within the build directory. The fact that it means copying over the package.json, readme, and license files may actually turn out to be an asset, at least in the case of package.json, as it will allow us to change the package name just for one flavor of the build, which could be the key to publishing both a current ES6 and a legacy ES5 version of the browser package in v7.

[Lms24]

Updated the PR description with current status. @lobsterkatie I think this PR is ready for review

[Lms24]

This will probably need to be rebased after #4699 is merged

[lobsterkatie]

This looks good!

The only thoughts I have are about the postbuild script.

1. Have you been testing this using npm, by any chance? I ask because I think this won't work with yarn. From their docs:

   Note that we don't support every single lifecycle script originally present in npm. This is a deliberate decision based on the observation that too many lifecycle scripts make it difficult to know which one to use in which circumstances, leading to confusion and mistakes. We are open to add the missing ones on a case-by-case basis if compelling use cases are provided.

   In particular, we intentionally don't support arbitrary pre and post hooks for user-defined scripts (such as prestart). This behavior, inherited from npm, caused scripts to be implicit rather than explicit, obfuscating the execution flow. It also led to surprising executions with yarn serve also running yarn preserve.

   I think just adding a && bash <path-to-script> to the end of the build command oughta do it, though. (And, IMHO, is actually nicer for exactly the explicitness reasons they mention above.)

2. Given that this is going to be used by all packages (eventually), I think the script file should live in the top-level scripts folder, even now when it's only being used by one package, because it's just one less thing to have to change later.

EDIT: Okay, I lied. I have one more thought. Given that all of our bundles start with the word bundle, what would you think of calling the folder bundles instead of bundle? It's not a huge difference, but I do think it makes it a little easier to parse at a glance if all of the path segments are different.

[lobsterkatie]

FYI, I made two edits up at the top of this PR:

• Your strikethrough wasn't working because of a missing space, so I added it.
• I changed the PR title to make it a ref rather than feat and to capitalize the first word after the colon.

[Lms24]

Thanks for the review!

1. Have you been testing this using npm, by any chance?

I've always tested the script with yarn (run) build and postbuild.sh was executed successfully. Also, CI executes my script successfully. I checked the link and saw that the documentation doesn't mention postbuild though which is kind of weird...

I think just adding a && bash <path-to-script> to the end of the build command oughta do it, though. (And, IMHO, is actually nicer for exactly the explicitness reasons they mention above.)

Makes sense and considering that postinstall isn't mentioned in the docs (but works anyway for some reason), I think it's better to && it to the build script. Also, it gives us the option to execute different postbuild scripts for multiple packages (thinking about angular-legacy 👀 ). I'll make the change.

2. Given that this is going to be used by all packages (eventually), I think the script file should live in the top-level scripts folder, even now when it's only being used by one package, because it's just one less thing to have to change later.

Fair point. I was thinking about this as well but decided to leave it for now in the browser package and to move it to the top-level once I changed the build directory in multiple packages. But I'll go ahead and move it now.

EDIT: Okay, I lied. I have one more thought. Given that all of our bundles start with the word bundle, what would you think of calling the folder bundles instead of bundle? It's not a huge difference, but I do think it makes it a little easier to parse at a glance if all of the path segments are different.

Sure, sounds good. I was also contemplating that but for some reason (which I can't recall atm) I left it at bundle. I'll change it though, as it makes much more sense in the plural, agreed.

[lobsterkatie]

I've always tested the script with yarn (run) build and postbuild.sh was executed successfully. Also, CI executes my script successfully. I checked the link and saw that the documentation doesn't mention postbuild though which is kind of weird...

That is super puzzling. The "it's just not in the docs" theory occurred to me, too, so I checked their codebase for postbuild and came up empty. That's what gave me confidence to say "nope, won't work." Given that we're changing it I suppose it doesn't matter, but... 🤔

[lobsterkatie]

I think this is in a good place! Once #4710 goes through, let's rebase and 🚀.

[lobsterkatie]

Suggested change

	# modify volta extends path
	# remove one `../` from the extension path of volta
	voltaExtendsCommand="/\"extends\": \"\.\.\/\.\.\/\.\.\/package.json\"/s/\.\.\/\.\.\/\.\.\//\.\.\/\.\.\//"
	# sed command to modify volta extends path
	# remove one `../` from the extension path of volta
	voltaExtendsCommand="/\"extends\": \"\.\.\/\.\.\/\.\.\/package.json\"/s/\.\.\/\.\.\/\.\.\//\.\.\/\.\.\//"

My internal monologue: Wait, I'm not quite sure I get what's happening here. Is he setting a variable or running sed? Ohhhh, this is a sed command. That's all inside the string. Riiiiiiiiight. Okay, I get it.

(When I first looked at the file, my eye was drawn immediately to the backslash-and-period extravaganza happening on this line, and so I hadn't yet read the comment above entryPointsCommand. Whoops. Still, can't hurt to be crystal clear.)

[Lms24]

I know, this regex is reaaally hard to read. Was sort of cringing as I was writing it out but I couldn't come up with a cleaner solution. Totally open for suggestions though.

[lobsterkatie]

I think the regex is what it is. I definitely think the second comment is needed, though!

EDIT: Hang on, though. Are we even sure we need this? What happens if you just delete the volta config all together? Now that I'm thinking about it, its only job is to tell volta which versions of node and yarn should be used when working in sentry-javascript, not when using @sentry/browser.

EDIT of the EDIT: Actually, I'd first try just not touching it, because that's even easier than deleting it. (And I think this will work, at least as well as changing it, because once the package is installed in a user's node_modules folder, that path will be meaningless anyway, regardless of whether it's the old or new value.)

[Lms24]

I went with not touching it at all for now, as it didn't seem to make a difference. I was thinking that the volta path needed adjustment for the pack script but it seemed like that wasn't necessary. Good catch, thanks

[lobsterkatie]

FYI: For my bundling work, it turned out to be better to split type-generation into a separate tsc call rather than doing it programmatically through a rollup plugin, and I've done that in #4724. (Turns out there's no way to make it happen exactly once per package if you're using rollup to generate more than one build, because all plugins run once per config rather than once per set of configs as I'd hoped.)

I've pulled your types changes in my suggestions below, so that the PRs can be merged in either order.

[Lms24]

I've pulled your types changes in my suggestions below, so that the PRs can be merged in either order.

Awesome, I'll apply the suggestions, rebase and merge this PR first thing tomorrow then. Thanks

[lobsterkatie]

Awesome, I'll apply the suggestions, rebase and merge this PR first thing tomorrow then. Thanks

Do give the volta thing a shot also before you merge, please!