chore: updates to dev-deps and QOL improvements #918
Conversation
commit: |
edodusi
force-pushed
the
chore/updates-and-qol
branch
from
84e4410 to
2d50232
Compare
There was a problem hiding this comment.
Hi @edodusi I left couple of question regarding changes on the builds
| @@ -34,7 +34,7 @@ const bundles = [ | |||
| ;(async () => { | |||
| for (const bundle of bundles) { | |||
| await build({ | |||
| configFile: false, | |||
| configFile: 'vite.config.ts', | |||
There was a problem hiding this comment.
Hi @edodusi why this change?
There was a problem hiding this comment.
@alvarosabu because otherwise our vite config is ignored in our build (eg. no types)
There was a problem hiding this comment.
So no types were generated before? Let's add this to the fix PR I requested below, I'm not convinced we need to add the whole vite.config.ts, there is a reason why we have a separate build script, otherwise,e we would be using one config instead. We can add the dts plugin directly here
There was a problem hiding this comment.
@alvarosabu I responded on the other comment about the vite.config, but to better clarify this point, we are using a custom file for the build because here we want to build 4 different bundles in parallel.
In the vite.config we just add a banner and the dts plugin. We could do it here, sure, but why? Passing a config file is normal practice for the build command, do you see any reason why we should ditch the config file and instead add inline configs here?
|
|
||
| export default defineConfig(() => ({ | ||
| plugins: [ | ||
| dts({ | ||
| insertTypesEntry: true, | ||
| outDir: 'dist/types', |
There was a problem hiding this comment.
Hi @edodusi why this change?
There was a problem hiding this comment.
@alvarosabu because this is how the exports are configured in the package.json, otherwise the paths are wrong
There was a problem hiding this comment.
hI @edodusi thanks for clarifying, then this should be in a separate PR marked as a fix. Is there a ticket for the typescript issues?
There was a problem hiding this comment.
@alvarosabu to be clear, this is not a fix, the build stays the same, with files in the dist and types in the dist/types folder. This and the above change just ensure that we use, according to the refactor previously made and following the practice of other modules, the dts plugin in vite.config
Without this and the above fix, we just use tsc to generate types, ignoring vite config and dts. This change just normalizes the config.
Let me know if you still think a fix release is needed.
There was a problem hiding this comment.
This is the build without the changes on this PR
This is the build with the changes
This means that the type directory wasn't generated correctly before.
There was a problem hiding this comment.
Installing storyblok-js-client on a project does have the correct structure, meaning that we probably introduced a bug in #858 but since we haven't released it since then users haven't been affected.
There was a problem hiding this comment.
@alvarosabu on the main branch (latest release) the build structure is the same, including types:
user@host in ~/storyblok-js-client (main)
$ ll -og dist
total 168
-rw-r--r--@ 1 28K Jan 10 14:54 index.mjs
-rw-r--r--@ 1 19K Jan 10 14:54 index.umd.js
-rw-r--r--@ 1 11K Jan 10 14:54 richTextResolver.mjs
-rw-r--r--@ 1 7.9K Jan 10 14:54 richTextResolver.umd.js
-rw-r--r--@ 1 5.0K Jan 10 14:54 schema.mjs
-rw-r--r--@ 1 3.1K Jan 10 14:54 schema.umd.js
drwxr-xr-x@ 12 384B Jan 10 14:54 types
| @@ -34,7 +34,7 @@ const bundles = [ | |||
| ;(async () => { | |||
| for (const bundle of bundles) { | |||
| await build({ | |||
| configFile: false, | |||
| configFile: 'vite.config.ts', | |||
There was a problem hiding this comment.
So no types were generated before? Let's add this to the fix PR I requested below, I'm not convinced we need to add the whole vite.config.ts, there is a reason why we have a separate build script, otherwise,e we would be using one config instead. We can add the dts plugin directly here
|
|
||
| export default defineConfig(() => ({ | ||
| plugins: [ | ||
| dts({ | ||
| insertTypesEntry: true, | ||
| outDir: 'dist/types', |
There was a problem hiding this comment.
hI @edodusi thanks for clarifying, then this should be in a separate PR marked as a fix. Is there a ticket for the typescript issues?
Pull request type
Updates and QOL improvements