-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add vite support #285
base: master
Are you sure you want to change the base?
Add vite support #285
Conversation
🦋 Changeset detectedLatest commit: ff0ab35 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 |
@michaeltaranto this is ready for review! ran cypress tests locally and they are passing |
987c0d3
to
e8210fd
Compare
@michaeltaranto @askoufis any updates here? 👀 I just rebased the PR as it had some conflicts |
@michaeltaranto @askoufis following up here! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the amazing effort! 👏
I gave this a crack a while ago, but didn't get too far. IMO the ultimate version of vite support for playroom would be a plugin, rather than a separate CLI + devserver, though that would likely involve a complete re-architect of playroom, so this is a very welcome addition in the meantime.
Just some minor comments from me.
@askoufis would you elaborate more on this plugin idea? (I am guessing it means there would be a plugin system and there would be webpack, vite plugins and so on) Also thanks for the review! gonna get to attending it this weekend! |
Yeah something like that. Basically, rather than playroom being a CLI that spins up a dev server, it would be a plugin that would integrate with bundlers like webpack and vite, using their dev servers to host the playroom site. |
@askoufis love that idea, agree on getting this in as a temporary solution and I'd love to get more involved in that 👀 |
81dfe2b
to
a41c296
Compare
@askoufis attended comments! still need to do some more testing, will also add the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Just a few more small comments.
if (typeof __webpack_public_path__ !== 'undefined') { | ||
__webpack_public_path__ = '../../../'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning behind this addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting in this together. I would like to see this progress, but really feel like this would be easier to support if it were broken into separate PRs.
As I see it we have:
- Migrate internals to TS
- JSX support
- Vite support
Currently the utils
refactor breaks a public (not documented) API:
import { createUrl } from 'playroom/utils';
We use this to build Playroom links on the Braid docs site.
Also would be keen to explore not having the bundler
config, and using the presence of webpackConfig
or viteConfig
as the decision point. This would avoid breaking existing configs too.
86b8065
to
ff0ab35
Compare
Thanks very much @TheMightyPenguin ! |
Description
Adds vite support. The motivation here:
Changes mode.
This is a chunky PR (sorry about that), here's an overview of the changes:
bundler
option to playroom config. This option is required, and should be either'vite'
or'webpack'
.viteConfig
option to playroom config. This is a function that returns the user Vite configuration.