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

Fix duplicated source code in built libraries #324

Merged
merged 26 commits into from
Nov 22, 2024
Merged

Conversation

lukehb
Copy link
Contributor

@lukehb lukehb commented Nov 21, 2024

Relevant components:

  • Signalling server
  • Common library
  • Frontend library
  • Frontend UI library
  • Platform scripts

Problem statement:

There are two problems causing this issue:

  1. Bundling too early.
  2. Hacky use of npm link to create packages within this repo that are shared (e.g. pixelstreaming-common) .

I'll expand on each of the problems in order.

Bundling too early

Until now, each of the libraries within this repo has been produced as a bundled and minified js file (using webpack). However, as there is now interdependencies between libraries and chains of dependencies within this repo, this repeated bundling/minification has resulted in transitive dependencies that were not able to be easily "tree shaken" and more importantly, ended up containing multiple copies of the same source that had been bundled and minified at different steps.

A good example of this was the implementations/typescript package which had a dependency chain something like this:

implementation/typescript depends on frontend-ui and frontend
frontend-ui depends on frontend

We ended up bundling frontend code into frontend-ui library, then when we built implementation/typescript, we depended on it and bundled it again.

Hacky use of npm link

We needed a way to specify which version of our libraries we (and our users) should depend upon when they use our published NPM packages but we also want to use our local packages when developing. Until now, we achieved this through specifying the dependency in the package.json and if we were working locally, using npm link ./SomeLocalFolder to override the download NPM package of the same name. Of course this was hacky, error prone, and a bit of maintainence headache for our CI.

Solution

Solution to bundling too early

This PR solves this problem by simply not doing any bundling/minification until the final application build step, in this example the final build step is implementation/typescript. But in user code, this would be your application that depends on the frontend and frontend-ui libraries.

This means bundling and minification is left to the user, but this seems reasonable and standard practice.

Instead of bundling our libraries (e.g. common, frontend, and frontend-ui), they are simply built and shipped as a NPM package that contains support for: es6 module usage (with source maps), commonjs usage (with source maps), and includes all the type definitions.

Solution to sharing packages for local dev and publishing

The solution to using local packages during development and publishing a clear, versioned, dependency for our libraries is to use an inbuilt feature of NPM, called NPM workspaces.

NPM workspaces allows us to:

  • Specify each of packages a workspace in our monorepo
  • Dependencies are treated as normal in each of our libraries
  • When we dev locally, the existence of the package.json workspace file in the repo root means NPM will try to resolve our dependencies using the local "workspace" directories first then fall back to the published packages.
  • This allows us to publish the packages with versioned dependencies as normal.
  • This allows us to completely remove the npm link hacks and just let npm resolve the dependencies when we work locally.

Documentation

Each library within the repo has had its readme.md updates accordingly. Additionally, the readme.md inside the repo root has been updated.

Test Plan and Compatibility

I have tested this change in all of our Github actions, tests, and across browsers in Chrome, Firefox, and Safari. This PR involved fixing some failing tests as a result and modifying some source files as a few things were written depending on the libraries being built in this way (or this change surfaced clashing dependencies).

I also added some new Github actions that test building each of packages using only the published libraries (as opposed to what is in the repo), this allows us to mimic what our users will experience and allows us to capture cases where we have forgotten to publish an NPM package update.

@lukehb lukehb merged commit a7e4a3e into master Nov 22, 2024
17 checks passed
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.

1 participant