fix: Build breaking after adding bun adapter #6059
Conversation
I tried the existing utility (@typescript-eslint/utils) for merging, but that balloned the cli.js to 110k lines, so I chose to use lodash for the merge, as it was already a peer dependency. I am sorry if there was another merging utility already in place, I didn't find it :( fix #6058
👷 Deploy request for qwik-insights pending review.Visit the deploys page to approve it
|
Twiggeh
changed the title
Twiggeh
changed the title
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "compilerOptions": { "types": ["bun-types"] } | |||
There was a problem hiding this comment.
To prevent the extra dependency we can define all the "types" (dafault + bun ) in the bun tsconfig.json
This revert some of your changes as well
There was a problem hiding this comment.
Or types can be moved into a .d.ts file instead, like in Nuxt or Remix and then referenced in includes field. This way typescript will be able to pick types from $PWD/node_modules/@types. Also, this way TypeScript should pick bun types from @types/bun package without any additional configuration
See the documentation: https://www.typescriptlang.org/tsconfig#types
If types field specified, then TS won't pick anything that is not included in the list:
This tsconfig.json file will only include ./node_modules/@types/node, ./node_modules/@types/jest and ./node_modules/@types/express. Other packages under node_modules/@types/* will not be included.
So my suggestion is to create a file, let's call it qwik.env.d.ts for example, where we reference vite/client types, and then add it to the includes.
The template would look like this:
In qwik.env.d.ts:
/// <reference types="vite/client" />Then in tsconfig.json:
{
"include": ["qwik.env.d.ts", "**/*.ts", "**/*.tsx"]
}Then for those who want to keep using bun-types (as they might be updated more frequently, aligned with the runtime releases) instead of @types/bun, they can just remove @types/bun after they installed the adapter and reference bun-types same way in qwik.env.d.ts
| @@ -97,6 +98,7 @@ | |||
| "execa": "8.0.1", | |||
| "express": "4.18.3", | |||
| "install": "^0.13.0", | |||
| "lodash": "^4.17.21", | |||
There was a problem hiding this comment.
Is there no smaller dependency? Or just few lines of helper code?
At the very least you should use lodash-es
| @@ -86,11 +87,15 @@ async function mergeJsons(fileUpdates: FsUpdates, srcPath: string, destPath: str | |||
| try { | |||
| const srcPkgJson = JSON.parse(srcContent); | |||
| const destPkgJson = JSON.parse(await fs.promises.readFile(destPath, 'utf-8')); | |||
| Object.assign(srcPkgJson, destPkgJson); | |||
| const mergedJson = mergeWith(srcPkgJson, destPkgJson, (value, srcValue) => { | |||
| if (Array.isArray(value)) { | |||
There was a problem hiding this comment.
This seems dangerous to do for unknown fields
Overview
What is it?
Description
Issue
When running the adapter for adding bun, it misses adding the types to
tsconfig.json(this is also not detailed in the documentation for bun). When these types are missing the build fails.I used lodash to do a deep merge over the jsons, I am sorry if there was another merging mechanism in place. I only found
@typescript-eslint/utilswhich had a deepMerge, but that balooned the output of the cli.js to over 110k lines, so I opted to uselodashinstead, which already is a peer dependency.Use cases and why
Checklist: