Skip to content
This repository was archived by the owner on Oct 23, 2023. It is now read-only.

Conversation

@TimKolberger
Copy link
Collaborator

@Shyrro Thank you for your great work in #78!

I updated the implementation to

  • include autocomplete for props
  • be rebased onto main
  • have a streamlined clean code package config and tsconfig
  • include a changeset

Let me know what you think :)

I am not able to target this PR to your PR, because it is on a forked repo.

@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #81 (a96ac63) into main (dd062b4) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main       #81    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            3        10     +7     
  Lines          145       518   +373     
  Branches        10        39    +29     
==========================================
+ Hits           145       518   +373     
Impacted Files Coverage Δ
packages/preact/src/forwardRef.tsx 100.00% <100.00%> (ø)
packages/preact/src/index.ts 100.00% <100.00%> (ø)
packages/preact/src/polymorphic-factory.tsx 100.00% <100.00%> (ø)
packages/react/src/forwardRef.tsx 100.00% <100.00%> (ø)
packages/react/src/index.ts 100.00% <100.00%> (ø)
packages/react/src/polymorphic-factory.tsx 100.00% <100.00%> (ø)
packages/solid/src/index.ts 100.00% <100.00%> (ø)
packages/solid/src/polymorphic-factory.tsx 100.00% <100.00%> (ø)
packages/vue/src/index.ts 100.00% <100.00%> (ø)
packages/vue/src/polymorphic-factory.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Shyrro
Copy link
Contributor

Shyrro commented Dec 7, 2022

Hi @TimKolberger !

Thanks for taking the time to review and improve !

Overall, the changes look good to me.

Though i just think that in the compilerOptions, there should be jsx: preserve instead of jsx: react.
Otherwise there is a chance the the build output won't be correct !

Aside from that, the rest looks good to me and it's very cool that you got to improve autocomplete !

@TimKolberger TimKolberger force-pushed the feat/polymorphic-vue-autocomplete branch from 3e55040 to efdb143 Compare December 8, 2022 21:03
@TimKolberger
Copy link
Collaborator Author

Though i just think that in the compilerOptions, there should be jsx: preserve instead of jsx: react.
Otherwise there is a chance the the build output won't be correct !

With "jsx": "preserve" we would ship actual JSX in the bundle, e.g.

image

This would force our users to transpile this package again when building their app.
In my understanding this is not what we want to do - we want to ship "just JavaScript" and transpile our JSX to the jsx factory { h } from vue.

By using "jsx": "react" with "jsxFactory": "h", "jsxFragmentFactory": "Fragment" we are telling TS to use h instead of React.createElement, which leads to this bundled code:

image

I was one thing missing though to make it work completely, the injection of import { h, Fragment } from "vue"; into the final bundle, otherwise h would be undefined. Which I added manually by configuring esbuild with an inject script.

I'll merge this PR as is, and the automatic rc release will be triggered. We can test the release and make further adjustments before releasing the first version of @polymorphic-factory/vue.

@TimKolberger TimKolberger merged commit cc259c2 into main Dec 8, 2022
@TimKolberger TimKolberger deleted the feat/polymorphic-vue-autocomplete branch December 8, 2022 21:18
@github-actions github-actions bot mentioned this pull request Dec 8, 2022
@Shyrro
Copy link
Contributor

Shyrro commented Dec 8, 2022

Though i just think that in the compilerOptions, there should be jsx: preserve instead of jsx: react.
Otherwise there is a chance the the build output won't be correct !

With "jsx": "preserve" we would ship actual JSX in the bundle, e.g.

image

This would force our users to transpile this package again when building their app. In my understanding this is not what we want to do - we want to ship "just JavaScript" and transpile our JSX to the jsx factory { h } from vue.

By using "jsx": "react" with "jsxFactory": "h", "jsxFragmentFactory": "Fragment" we are telling TS to use h instead of React.createElement, which leads to this bundled code:

image

I was one thing missing though to make it work completely, the injection of import { h, Fragment } from "vue"; into the final bundle, otherwise h would be undefined. Which I added manually by configuring esbuild with an inject script.

I'll merge this PR as is, and the automatic rc release will be triggered. We can test the release and make further adjustments before releasing the first version of @polymorphic-factory/vue.

Uhm i see, i understand. Thank your for taking the time to fix this, i did not think about that !

Now that i think about it, if we want to ship JS we could probably just write this package without TSX and use plain TS with the h function. Would simplify things.
We would just have a tsconfig for test files to avoid rewriting them.

If we want to keep the TSX however, i do think that we should think of something else, because aside from the h & Fragment, Vue JSX transform is actually quite different from React JSX transform in multiple ways. Also, Vue docs also recommend to use jsx: preserve and let Vue JSX transform do the rest.

We could probably consider this : https://github.com/sxzz/unplugin-vue-jsx
There is an esbuild plugin that allows esbuild to perform the right transforms and will probably help us ship the right JS.

That being said, we will see how this works out and it might do fine given the low JSX complexity of the package.

Let's ship it and keep an eye on it !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants