Skip to content
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

Preact-Vite: Add framework #20390

Merged
merged 23 commits into from
Dec 28, 2022
Merged

Preact-Vite: Add framework #20390

merged 23 commits into from
Dec 28, 2022

Conversation

KrofDrakula
Copy link
Member

@KrofDrakula KrofDrakula commented Dec 23, 2022

Issue: #20052

What I did

Implemented Vite builder coupled with Preact renderer using the official @preact/preset-vite plugin.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

The implementation is still a WIP while I integrate all the necessary scaffolding for tests. It's now done.

Development notes

  • running Verdaccio requires running NPM 8 (and possibly Node 16) instead of latest, else npm adduser doesn't work

@KrofDrakula KrofDrakula marked this pull request as ready for review December 27, 2022 13:31
@KrofDrakula KrofDrakula requested review from shilman and IanVS December 27, 2022 13:31
@KrofDrakula
Copy link
Member Author

@shilman @IanVS The duplicated plugin problem that blocked the last two tasks has been resolved with Ian's suggestion in the Discord discussion. The stories now run in the generated sandbox, E2E tests pass and I've verified that I can run Storybook from within the sanbox and generate a new project and browse components there, too.

I've configured the TS compiler to match Preact's preferred configuration for JSX (react-jsx with preact being the import source), so projects shouldn't need to import h when using JSX, nor providing a // @jsx h header comment anymore. We may need to update the documentation to reflect this, too. Maybe even open another PR to double-check the Preact config in webpack to match this?

@shilman
Copy link
Member

shilman commented Dec 27, 2022

@KrofDrakula great stuff!! CI is failing though

image

Looks like the CLI is not auto-detecting the React project properly with these changes. Any idea what's going on?

@KrofDrakula
Copy link
Member Author

@shilman I know exactly what's going on. :) The CLI script uses RegExp to match the template name, and react-vite/default-js is a subset of preact-vite/default-js which is why it prompts to select an unambiguous template. This times out the test script after 10 min because it expects a resolution to the prompt.

Not sure how you'd like me to solve that? Seems like the matcher is intentional.

@shilman
Copy link
Member

shilman commented Dec 27, 2022

@KrofDrakula OOF. I'm not familiar with the code & didn't trace through in detail but could you do something like this?

const filterRegex = new RegExp(`^${filterValue}`, 'i');

Perhaps @yannbf has thoughts as well

@KrofDrakula
Copy link
Member Author

KrofDrakula commented Dec 27, 2022

@shilman I didn't want to change existing behavior (since it currently matches any JS/TS framework combo if using "js", for example), but I guess this is the first time this has to be considered since none of the other strings are subsets of each other. It would solve THIS problem, but not the example I mentioned.

Thoughts? Would we care if that expectation broke?

@shilman
Copy link
Member

shilman commented Dec 27, 2022

This is sort of an internal tool, i think a breaking change is fine. As for what the new behavior should be, you guess is as good as mine!

@KrofDrakula
Copy link
Member Author

@shilman If that's the case, I'm fine with matching with the start of the string. I wasn't aware if this was exposed to a wider audience. I guess just people that create reproductions, but they would just get prompted anyways without having to specify an explicit name.

Start of string match it is, then. /cc @yannbf

@KrofDrakula
Copy link
Member Author

@shilman Board is green, as far as I can tell, this is ready to be reviewed.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @KrofDrakula -- I made a minor change to fix outdated package deps, but otherwise LGTM

@shilman shilman changed the title Preact-Vite framework implementation Preact-Vite: Add framework Dec 28, 2022
@shilman shilman merged commit 160ce8a into next Dec 28, 2022
@shilman shilman deleted the preact-vite-framework branch December 28, 2022 16:15
code/frameworks/preact-vite/README.md Show resolved Hide resolved
code/frameworks/preact-vite/README.md Show resolved Hide resolved
code/frameworks/preact-vite/package.json Show resolved Hide resolved
code/frameworks/preact-vite/package.json Show resolved Hide resolved
code/frameworks/preact-vite/package.json Show resolved Hide resolved
code/frameworks/preact-vite/package.json Show resolved Hide resolved
code/frameworks/preact-vite/src/preset.ts Show resolved Hide resolved
@shilman
Copy link
Member

shilman commented Dec 28, 2022

@IanVS sorry i jumped the gun here -- @KrofDrakula can you please address the comments in a followup PR?

@KrofDrakula
Copy link
Member Author

@IanVS @shilman Sure thing. Will post a fix soon.

@KrofDrakula
Copy link
Member Author

@IanVS @shilman Done. #20432

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants