feat(react-output-target): generate functional components and ES modules #432
Conversation
Lerna/symlinks are causing a @types/react resolution issue where the dependency is not de-duplicated. These changes should not be needed. Tested outside of the mono-repo and deps resolve correctly.
sean-perkins
requested review from
alicewriteswrongs,
tanner-reits and
thetaPC
and removed request for
a team
There was a problem hiding this comment.
Overall, looking good. Noticed a couple things:
- The runtime entry-point isn't a valid import location in some instances. This is an issue with the React library's
tsconfig.jsonvalues. SettingmoduletoPreserveandmoduleResolutiontoBundlerresolves it. Need to determine if there are any breaking changes or downstream issues for projects that need to make this change. - Output is not cleared between builds. This can cause issues with non-existent components being left in the output directory when using
esModules.
|
I looked into 1. The use of Reflecting more on our conversation with 2, I am less confident if we should clear the directory or files. The goal is to co-locate your generated component wrappers alongside any potential React-only components/component implementations. Clearing the directory between builds would prevent this. If we wanted to do this is a less destructive way, we would need to keep a generated output of what the react-output-target generated somewhere and parse that to determine what files are "owned" by the output target and are safe to delete between builds. This could be a cool feature, but I would recommend we track that as follow-up scope. When testing I also observed the new output no longer uses the syntax sugar for bundlers like webpack for marking functions as pure. I will see what the work effort to generate this through ts-morph is. |
|
I think that's fine. We're making a similar concession in Stencil for generating export maps. Hard to know what was user-generated and auto-generated and what changed between builds. |
|
We received some feedback from testing, there is two different underlying issues with more complex type usages on custom events. I am going to try reproducing locally and update the output target to account for those. It is possible for it to use a reference as value instead of |
|
Identified the issues with the PR. I was incorrectly using the resolved (inline) type instead of directly leveraging the original type references that Stencil re-exports and specifies on the events metadata. This ended up resolving both issues. |
…les (#432) * feat: migrate to lit/react component wrappers
…les (#432) * feat: migrate to lit/react component wrappers
…les (#432) * feat: migrate to lit/react component wrappers
* feat(react-output-target): generate functional components and ES modules (#432) * feat: migrate to lit/react component wrappers * exploration nextjs support * update Stencil with support for DSD * get rid of hydration errors * enhance output target * add tests * fix dep * remove type property in package.json * remove hydrate dir * move ssr support into custom export * clear hydrate folder again * skip lib check * skip lib check * better resolve light dom * remove unused import * match default dir with stencil * more improvements on light dom rendering * remove duplicate hydrate ot * remove style prop * fail if outdir is not set * import hydration script and ssr runtime only within the server component * validate hydrate output target to be set if hydrateModule option is set * improve implementation * separate files for server and client components * properly create server and client side components * don't parse children * use own runtime * validate that external runtime is set to 'true' * adjust test * update Stencil dev build * recognise that externalRuntime default is true * bring back light DOM rendering for better hydration results * revert light dom approach * minor formatting * update stencil * explicitly type component exports * unit test tweak * fix build * use latest Stencil release * fix test --------- Co-authored-by: Sean Perkins <[email protected]>
I just ran into this issue trying to update to the latest version of this package. Would be great to document this in the README. :) |
|
@TRMW these details are documented in the README: https://github.com/ionic-team/stencil-ds-output-targets/tree/main/packages/react-output-target and on the official docs: https://stenciljs.com/docs/react#step-2---react-component-library |
|
@sean-perkins I just realized this! Thanks for the help though! |
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally for affected output targetsnpm test) were run locally and passednpm run prettier) was run locally and passedPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue URL: resolves #255, #89, #404, closes #361, #243, #198, #88, #359, #148, #138
The issues linked are either resolved from these changes or no longer apply due to these changes. I've tried to separate them as such to help future maintainers separate why so many tickets are attached to this PR.
What is the new behavior?
React Output Target
use client;directive.Does this introduce a breaking change?
Other information
Latest dev-build (2024-05-22):
0.0.1-dev.11716386285.1a6265fd