Conversation
🦋 Changeset detectedLatest commit: da661f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Generally i love this idea and its been something we wanted to look at. I am curious about the issues with |
|
IIRC one of the main problems was the favicon imports, bundling them and keeping them as imports in JS so the downstream webpack would pick them up properly was something I couldn't figure out how to configure. I can take another look at some point to remember if there were any other issues. |
3b054bb to
fb7f867
Compare
|
I pushed the tsdown build back up. The favicon stuff was actually simple enough, left it as external and copied the assets over so the imports would be resolvable. However, as is evident from the CI run, the editor tests fail when the source is bundled with tsdown. For some reason, e.g. the |
src/utils/componentsToHints.ts
Outdated
| // interop with esm build and cjs in jest | ||
| const parsePropTypes = typeof ppt === 'function' ? ppt : ppt.default; | ||
|
|
||
| const configComponents = (await import('../configModules/components')).default; |
There was a problem hiding this comment.
So I think I figured out the issue, at least to some extent - since the parse-prop-types import is side-effectful to some extent, it needs to be before this configComponents import as well after bundling. Note that this is also mentioned in the componentsToHints test file that it's side-effectful.
However, it seems that tsdown/rolldown reorders the imports so they swap places, which broke the prop types parsing functionality.
Maybe this is the issue, not sure: rolldown/rolldown#6436
Anyways, seems like changing this to be a dynamic import "works around" the problem, but it certainly is not ideal...
| @@ -1,3 +1,6 @@ | |||
| // Import as a side-effect here so parse-prop-types would patch "prop-types" before components import. | |||
| import 'parse-prop-types'; | |||
There was a problem hiding this comment.
Adding a side-effectful import of parse-prop-types here seems to have resolved the issue without the dynamic import in the previous commit. I'm not too happy with it, but also not aware of other options... at least it works?
| @@ -80,21 +80,12 @@ | |||
| "@babel/parser": "^7.23.4", | |||
| "@babel/preset-env": "^7.20.2", | |||
| "@babel/preset-react": "^7.18.6", | |||
There was a problem hiding this comment.
@michaeltaranto I'd appreciate if you could take a hard look at the dependencies, if what I moved to devDeps makes sense, and if anything else could potentially be moved. A bit hard for me to judge, as I don't know what has been added and why.
Also the webpack config and loaders, which I trimmed down - maybe something can be simplified further there as well.
There was a problem hiding this comment.
I dont recall why we have a requirement for @babel/core (unless its a peer dep of something we use).
The @babel/preset-react package is part of the default webpack config we provide for consumers, so we should add that back in.
Can all the @types/* packages be moved to devdeps now?
There was a problem hiding this comment.
@babel/core is indeed a peerDep requirement for all other babel packages
There was a problem hiding this comment.
I hadn't moved @babel/preset-react, so that should be good already.
Types can indeed move to devDeps, will do so.
tsdown.src.config.ts
Outdated
| platform: 'browser', | ||
| plugins: [vanillaExtractPlugin()], | ||
| // Doesn't affect the bundle but suppresses a warning we don't care about | ||
| copy: [{ from: 'images', to: 'dist' }], |
There was a problem hiding this comment.
Thoughts on copying over the template.html to the app directory? That way the makeWebpackConfig file only operates on the dist/app directory (can pull the resolution of this path into a variable to be shared between all references in that file).
There was a problem hiding this comment.
Oh yes, that's a good idea - I also noticed I hadn't changed files in package.json yet. Now we can remove src from the dist altogether, as everything is contained in app.
tsdown.app.config.ts
Outdated
There was a problem hiding this comment.
Just thinking through the design of the output directory. I think ideally in the future we would have two parts, the "cli" and the "app". Currently the "cli" is the "lib" folder, so that makes what we are working on here the "app".
A few suggested tweaks:
- Rename "dist/src" to "dist/app"
- Output entries in their own "entries" folder
There was a problem hiding this comment.
It's easy enough to change afterwards, as the added app bundle directory is not part of the public API.
I switched to a separate top-level app directory - that makes things a little bit easier actually, as we no longer have to copy the images over - they are automatically resolvable from a top-level directory.
There was a problem hiding this comment.
Im not sure i agree with the top-level app directory, feel like its clearer to have all compiled output under the parent dist directory
There was a problem hiding this comment.
Might've misunderstood :) Moved everything back to dist, but split it so app goes into dist/app and the previous utils build now goes into dist/utils. I also added back the copying of images and removed the root images dir from dist files, so we wouldn't ship them twice.
Is it better now? 😄
| @@ -1,6 +1,5 @@ | |||
| { | |||
| "presets": [ | |||
| "@babel/preset-typescript", | |||
There was a problem hiding this comment.
Hmm, removing this makes jest fail. But this file is part of distributed files, so it may get used by webpack at some point? We wouldn't install the typescript preset for users anymore, so if it's used someway, removing it feels more correct.
Should it be removed from distributed files, or should we provide a separate babel config for jest?
There was a problem hiding this comment.
Looks like all internal babel uses provide a configuration explicitly, so I don't think it's used, and I'd remove it from dist files.
There was a problem hiding this comment.
Removed then. I also removed utils dir from dist files, I think it's some old remnant there, as utils are already compiled to dist and wouldn't be accessible from there anyways due to package#exports.
| "scope-eval": "^1.0.0", | ||
| "sucrase": "^3.35.1", | ||
| "tinyglobby": "^0.2.12", | ||
| "typescript": ">=5.0.0", |
There was a problem hiding this comment.
moved typescript back to deps as it's used by getStaticTypes via makeWebpackConfig, so would probably error in a clean project.
might be worth considering to make that logic conditional on the consumer having typescript installed in the future.
Note: this is just me playing around and poking the bear to see if you're interested in this.
Currently, playroom ships the entire src directory as-is, meaning that the job of compiling playrooms internal components and styles, etc falls to the user. This also means that there are quite a few dependencies included just because of this, mainly a bunch of Vanilla Extract related deps.
What if playroom would prebundle its source code, compiling out the TypeScript and Vanilla Extract code, and leaving just normal CSS/JS to be imported by webpack?
This cleans up a bunch of code and deps - they can be devDeps now. Maybe some more can be move, e.g. some of the
@types/*packages? The webpack config is quite complex, I'm not exactly sure how much of that was just needed to compile src previously.Note: the src is bundled with vite library mode instead of tsdown as I ran into some weird issues with tsdown - but maybe we can figure out some solution if you think using vite for build is not the best idea.