Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

ref(build): Add central build directory from JS SDK#321

Merged
Lms24 merged 4 commits intomainfrom
lms-build-dir
Nov 21, 2022
Merged

ref(build): Add central build directory from JS SDK#321
Lms24 merged 4 commits intomainfrom
lms-build-dir

Conversation

@Lms24
Copy link
Copy Markdown
Member

@Lms24 Lms24 commented Nov 18, 2022

This PR makes a change to the build process: Previously, transpiled JS, declaration files and source maps were written into dist. To align Replay with the build process in the SDK monorepo, this PR makes the following changes:

  • Change build output to build/npm (see explanation below)
  • Change build:npm command to execute a prepack script
  • Temporarily introduce the ./scripts/tmp-prepack.ts script which copies files like README.md, LICENSE and package.json into build/npm. This is a simplified version of the monorepo's prepack script. While doing so, the script makes changes to package.json to align the entry points.
    • This file will be removed once we migrated as we can then use the monorepo's prepack script.
  • Remove the files entry in package.json for consistency with the monorepo (we just rely on .npmignores)
  • Adjust occurrances of dist folder, replacing or adding build as appropriate

As of this PR, the build directory will have the following structure:

<replay-root>/
├─ build/
│  ├─ npm/ <-- everything in this directory goes into the NPM tarball
│  │  ├─ types/
│  │  │  ├─ *.d.ts files (+maps)
│  │  ├─ index(.es)?.js file + map
│  │  ├─ package.json
│  │  ├─ LICENSE
│  |  ├─ README.md
├─ ...

More context on the JS SDK's build directory structure:

While this is arguably not the simplest setup, the structure below serves two purposes:

  1. Have one directory per SDK into which all build and artifact (tarball, CDN bundle) related files go
  2. Be able to make modifications to the files we ship vs. the ones we keep in the repo.
    For example, we remove certain properties in the shipped package.json, which our users don't need

Once we finished the migration of Replay, the build directory structure will be identical to the JS SDK structure of SDK packages with bundles:

<replay-root>/
├─ build/
│  ├─ bundles/ <-- that's where the CDN bundles will live
│  ├─ npm/ <-- everything in this directory goes into the NPM tarball
│  │  ├─ types/
│  │  │  ├─ *.d.ts files (+maps)
│  │  ├─ esm/
│  │  │  ├─ ESM JS files (+maps)
│  │  ├─ cjs/
│  │  │  ├─ *CJS files (+maps)
│  │  ├─ package.json
│  │  ├─ LICENSE
│  |  ├─ README.md
├─ ...

Note that as soon as we change the entry point paths in package.json (main, modules, types), we're technically introducing a breaking change. It is however a very edge-case-y scenario in which users import modules from an explicit path rather than from the top-level package:

// this import would break after moving the index file to esm/index.es.js
import {} from '@sentry/replay/index.es';

// this import would still work as expected
import {} from '@sentry/replay';

I would argue that introducing this breaking change is fine as long as we're in this 0.x/alpha state but I'm happy to hear other opinions on this.

As of this PR nothing breaking is happening yet.

@Lms24 Lms24 requested review from billyvg and mydea November 18, 2022 13:34
@billyvg
Copy link
Copy Markdown
Member

billyvg commented Nov 18, 2022

Thanks for the detailed explanation -- I agree with you in regards to the breaking change.

Copy link
Copy Markdown
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

Very nice! 👍

@Lms24 Lms24 force-pushed the lms-pkgjson-scripts branch from f371bcf to 6a73681 Compare November 21, 2022 13:12
Base automatically changed from lms-pkgjson-scripts to main November 21, 2022 13:15
Co-authored-by: Billy Vong <billyvg@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 21, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/index.js 0 B (-100% 🔽) 0 ms (-100% 🔽) 0 ms (-100% 🔽) 0 ms
build/npm/index.js 34.88 KB (+100% 🔺) 698 ms (+100% 🔺) 71 ms (+100% 🔺) 769 ms

@Lms24
Copy link
Copy Markdown
Member Author

Lms24 commented Nov 21, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/index.js 0 B (-100% 🔽) 0 ms (-100% 🔽) 0 ms (-100% 🔽) 0 ms
build/npm/index.js 34.88 KB (+100% 🔺) 698 ms (+100% 🔺) 71 ms (+100% 🔺) 769 ms

I guess we can't simplify this transition for size-bot 🤔

@Lms24 Lms24 merged commit b01303c into main Nov 21, 2022
@Lms24 Lms24 deleted the lms-build-dir branch November 21, 2022 13:38
mydea pushed a commit to getsentry/sentry-javascript that referenced this pull request Nov 23, 2022
…ry-replay#321)

Introduce central `build` directory to the Replay package:

* Change build output to `build/npm` (see explanation below)
* Change `build:npm` command to execute a prepack script
* Temporarily introduce the `./scripts/tmp-prepack.ts` script which
copies files like `README.md`, `LICENSE` and `package.json` into
`build/npm`. This is a simplified version of the monorepo's prepack
script. While doing so, the script makes changes to `package.json` to
align the entry points.
* This file will be removed once we migrated as we can then use the
monorepo's prepack script.
* Remove the `files` entry in `package.json` for consistency with the
monorepo (we just rely on `.npmignore`s)
* Adjust occurrances of `dist` folder, replacing or adding `build` as
appropriate

As of this patch, the build directory will have the following structure:
```
<replay-root>/
├─ build/
│  ├─ npm/ <-- everything in this directory goes into the NPM tarball
│  │  ├─ types/
│  │  │  ├─ *.d.ts files (+maps)
│  │  ├─ index(.es)?.js file + map
│  │  ├─ package.json
│  │  ├─ LICENSE
│  |  ├─ README.md
├─ ...
```

Co-authored-by: Billy Vong <billyvg@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants