Fix issues with module output #77
Conversation
tsconfig.json
Outdated
| "src/*" | ||
| ] | ||
| } | ||
| "emitDeclarationOnly": true, |
There was a problem hiding this comment.
Why emit declaration only here? Are we not relying on TS anymore for that, and are relying on vite? I don't really know anything about vite but I'd be happy to look into it, but from the config below it doesn't appear to me that it takes up the mantle of transpilation (although I could be wrong!)
There was a problem hiding this comment.
I did make it so that TS isn't the sole program responsible for creating the output bundles. With this change, TS would still be used for generating all the type definitions on the library, but Vite would be build the actual code.
There was a problem hiding this comment.
See my comment below, but I guess my understanding of this issue is that we probably don't need to bundle @formio/vue - we just need to optimize our codebase (as you have done by providing an ESM entry) so that downstream client application bundlers (such as vite, webpack, rollup, esbuild, etc.) can properly bundle this as a dependency. I could be missing something here, though, what are your thoughts?
There was a problem hiding this comment.
What's nice about using a bundler like Vite, Webpack, or Rollup is that they're only a part of how the library itself gets bundled. The applications that use @formio/vue don't need to use the same bundler (or any if they want to make their application work without one).
If you're unsure about it though, I'd be happy to spin up a simple Vue.js app that uses this fork but that doesn't use Vite as a proof of concept.
There was a problem hiding this comment.
Correct me if I'm wrong but this PR would provide a new entry point to the library (via the "module" parameter in package.json) that is the output of the vite bundler. So when other bundlers bundle this dependency, they'll be getting an already bundled output file, which in my experience with webpack can cause issues. I could be wrong though!
There was a problem hiding this comment.
I haven't personally experienced any issues with libraries that were bundled unless I didn't configure things right, but I know a lot of packages that bundle their code like React, Vue, and SweetAlert2. All of these packages use Rollup to bundle their libraries which is also what Vite uses internally for a production build (and what it uses in this PR).
There was a problem hiding this comment.
Yeah, sorry if I wasn't clear here, I would expect basically every library to bundle their code. The concern is not the bundling itself, it's the act of setting the bundled output as the entry point for downstream applications (so, for downstream bundlers). In any case, I just need to find the time to spin up a regular old vue application and see if there's any damage done
There was a problem hiding this comment.
@brendanbond Just to make sure things were working as expected, I put together a repository with a simple Vue application to test things. If you wanted to test it further, you could always use the repo as a base or a reference.
The main branch tests uses Vite, which is what's recommended by Vue for new projects. The webpack branch tests it using vue-cli-service. From my testing, everything works great 😄
There was a problem hiding this comment.
Awesome, thanks so much! I'll check it out ASAP and get this merged
There was a problem hiding this comment.
@brendanbond Just following up. Have you had a chance to look over things?
|
@kitsune7 also can you give us a high-level overview of what this accomplishes? If I understand it correctly, you're trying to use vite in a client application that runs formio and running into issues; is that correct? If that's the case, isn't introducing vite to this project overkill? In other words, it seems like providing an ESM entrypoint is enough that any downstream vite instance can properly bundle this; can you confirm? |
Using I understand the hesitancy to bring in a new bundler though, especially if you're not as familiar with it. |
|
Yeah my primary concern here is that introducing a bundler at this stage may break client applications that don't rely on vite (I can imagine that certain consumers of this library use, for example, Webpack or Rollup or something to bundle their front end dependencies) You're not the first person to point out this problem with single-spa though; I wonder if we can test by just creating a vanilla vuejs app that doesn't rely on vite and see if it successfully bundles |
| "experimentalDecorators": true, | ||
| "noEmit": true, | ||
| "noImplicitAny": false, | ||
| "skipLibCheck": true, | ||
| "strict": true, | ||
| "strictNullChecks": false, | ||
| "target": "es5", | ||
| "module": "commonjs", | ||
| "moduleResolution": "node", | ||
| "sourceMap": true, | ||
| "emitDecoratorMetadata": true, | ||
| "experimentalDecorators": true, | ||
| "declaration": true, | ||
| "lib": ["es7", "dom"], | ||
| "strict": true, | ||
| "skipLibCheck": true, | ||
| "types": ["node"], | ||
| "typeRoots": ["node_modules/@types"], | ||
| "baseUrl": ".", | ||
| "outDir": "lib", | ||
| "paths": { | ||
| "@/*": [ | ||
| "src/*" | ||
| ] | ||
| } | ||
| "types": ["node"] |
There was a problem hiding this comment.
Just to give some context to these changes, I made an update so that Vite/Rollup handles bundling the types, which eliminates the need for many of these properties. The tsconfig.json is obviously still needed for type checking, but I removed properties that dealt specifically with how Typescript would emit JavaScript and type declaration files. There are still some that I included such as target because Rollup takes the property into account when it's defined.
|
@kitsune7 thanks so much for taking the time to set up that toy repo. I will put it on my list for today and try to get this approved and merged. |
|
This is awesome! @brendanbond any update on if/when this could be merged in? |
|
@justingish mgmt wanted to double check w/ our other customers using Vue, I'm hoping this week |
|
@brendanbond got the approval, he will get it merged. |
The purpose of this PR is to solve some of the issues demonstrated by this repository. It accomplishes that by using Vite/Rollup to bundle everything into UMD and ES Module output files.
I'm happy to make any adjustments if there are any problems with these changes, but if you clone the repository that reproduces the issue and run the solution suggested at the bottom of the README, you'll find that this fixes the problem.