-
Notifications
You must be signed in to change notification settings - Fork 118
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lready built if not specified.
…a single lock in the repo root.
This was referenced Nov 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant components:
Problem statement:
There are two problems causing this issue:
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, usingnpm 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:
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.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.