Feat: Add Bundler and tighten Docker Image #17
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
This separates the TS files into `src/core` and `src/scripts` to differentiate between the files that are used in the main app and the standalone scripts that are run from the command line. On top of that, we will have a `src/config` folder for the config files. Since the files are ignored by git, we need to add a `.gitkeep` file to keep the folder in the repo. Lastly, to make the imports easier, we will add path aliases to the `tsconfig.json` file so we can drop the relative paths
This allows us to import the urls.json file directly, instead of having to read the file and parse it. To enable this we need to set the `resolveJsonModule` compiler option to `true`.
This bundler is a wrapper around esbuild, and allows us to easily bundle our code for Node as ESM. In addition, it allows us to better control over what we want to compile and will automatically import dependent modules for the entry points we specify. SWC, Rspack, Webpack, and vanilla esbuild were all considered, but tsup was the easiest to use and configure for the simple use case of compiling our ESM TS code for Node while still being in ESM format. On top of that, it has finer control over the entrypoints and automatically imports dependent modules. The build.js file is a simple wrapper around tsup that will allow us better control over the build process which will significant in later commits.
This update introduces yargs options to selectively build 'core' or 'scripts' files. This enhancement allows for targeted builds, saving time by avoiding unnecessary builds. Tsup's automatic handling of dependencies eliminates the need for direct transpilation of core files for script builds and verbose pathing to core and config files. This results in standalone output files, free from runtime external file dependencies. Furthermore, the script files are now output to a 'bin' directory, maintaining a clean 'dist' directory and indicating that the bin files are standalone scripts executable from the CLI and are not part of the main app.
For Docker builds, we don't want to bundle the config files into the dist/index.js file. By bundling them, we lose the ability to change them without rebuilding the image. Rebuilding the image every time we want to change the config files is not ideal and would prevent us from using the image for other sites. Solving this problem is a bit tricky because we want to use the same build script for both Docker and non-Docker builds, keep the independent core and scripts bundling, and retain the path aliasing functionality. To achieve all of this, I used the following: - tsup's 'external' option to exclude the "@config" modules from the bundle - The tsc-alias package to resolve the "@config" paths to a relative path - The process.env.DOCKER variable to determine whether to use tsup's 'external' option or not (this variable is set in the Dockerfile). By doing all of this, we can now pass the config files into the container as volumes and change them without rebuilding the image. At the same time, we can still use the same build script for local development and retain the simplicity of standalone builds. The use of tsc-alias was discovered in this issue: evanw/esbuild#394 (comment). Without it, "@config/*" paths would remain in the bundle and would cause errors as there is no way for "@config" to be resolved as a local path.
When we build the files for Docker, we will import the urls.json file locally instead of the contents being bundled into the file. This raises ERR_IMPORT_ASSERTION_TYPE_MISSING when we try to import the file. To resolve this issue, we need to add the `type: "json"` to the import statement as well as change the "module" and "moduleResolution" options in the tsconfig.json file as import assertions are only supported in 'esnext' or 'nodenext'.
These files are not meant to be used directly, but rather copied to specific directories and modified as needed. This is an attempt to make the root directory less cluttered.
These files are not meant to be used directly, but rather copied to specific directories and modified as needed. This is an attempt to make the root directory less cluttered.
dotenv will automatically remove quotes from values, but if we pass the .env file to the Docker container with the '--env-file' option, the quotes will be preserved. This will cause 403 Forbidden errors with the Datadog API because its interpreted the quotes as part of the value. The simplest solution is to add a comment to the .env.template and leave it up to the user to ensure they don't wrap values with quotes.
We now build a bin directory, so we need to ignore it. The config files, 'lh-config.js' and 'urls.json' will be volume mounted into the container, so we can ignore them. Lastly, we no longer want to include the .env file in the image, as we will be deploying the image to ECR and we don't want our DD keys in the image.
This should make it easier to get started with the project.
This script will run the docker container with the config files mounted and the .env file sourced. We can then run this script through pnpm.
|
dev_block ✊ |
|
un_dev_block ☁️ , @deltuh-vee try running it with |
|
Was index.js supposed to be moved out of the src directory? I still see it there. |
|
QA 🎬
|
batbattur
reviewed
Dec 4, 2023
batbattur
force-pushed
the
add-tsup-bundler
branch
from
c58a9d6 to
5a23f65
Compare
|
Sorry, accidentally pushed unrelated commit to the branch! Reverted to the last commit. CR 🚛 from #17 (review) |
mlahargou
approved these changes
Dec 7, 2023
| @@ -132,7 +133,7 @@ async function captureLighthouseMetrics(pageType: string, url: string, audits: s | |||
| console.log(`Done running Lighthouse for ${url} with form factor: ${formFactor}\n`); | |||
| } | |||
| (async() => { | |||
| let inspectList: Record<string, string[]> = JSON.parse(await fs.promises.readFile('urls.json', 'utf8')); | |||
| let inspectList: Record<string, string[]> = URLS; | |||
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.
Description
This has some major changes to the build logic and dev environment. The core logic remains the same.
Changes:
tsupandtsc-aliasto compile and bundle the code for specific uses and targetspathaliases in thetsconfig.jsonCR
The main goal of these changes were to allow us to have individual folders for our Typescript code -- i.e. config, core, and scripts. Then when we needed to execute the code, it would be properly compiled without nested folders in the
distfolder. To accomplish that, we needed a bundler that could parse all the Typescript code and compile it for Node as standalone JS files.I tried out different bundlers:
rspack,swc, andwebpack. They all had their own issues and I couldn't get them to work. This was mainly due to a Node issue with ES modules and CJS modules not being able to be mixed but also not being able to be separated when working on a Node app:evanw/esbuild#1921 (comment).
At the end of the day I only got
esbuildto work and in the desired way to address both #11 and #14. That said,esbuildstill had its own issues which led me to discover the tooltsup.tsupis a wrapper aroundesbuildand fixes some of the underlying issues of working with theesbuildAPI including shimming modules -- https://tsup.egoist.dev/#inject-cjs-and-esm-shims.Aside from that, we also need the ability to not include the config files -- i.e
lh-config.jsandurls.json-- in the bundleddist/index.jsfile in order to manually configure these options for the Docker container. If they were included then we would have to rebuild the Docker image every time we wanted to change the configs and would need to build and tag multiple images for variations in the configs. d9d6942 provides further details and explanation around this requirement.QA
setupscript workspnpm run setup.env,lh-config.js, andurls.json) have been copied to the appropriate locations.run-dockerscript worksREADMEsetup instructionsdocker build -t vigilo .pnpm run start:docker.envfile is not copied over to the docker imageREADMEsetup instructionsdocker build -t vigilo .docker run --rm -it vigilo /bin/bashlsto ensure the.envfile was not copiedstartscript still works - Reference the instructions from Port lighthouse-docker to TS in New Repo #1updateDatadogMetricsMetadatascript still works - Reference the instructions from Feature: Add Ability to Update Metadata for a Metric in Datadog #9createDashboardsscript still works - Reference the instructions from Add script to create dashboard #8Closes: #11
Closes: #14
cr_req 2