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

fix: move JSX DOM types back to @vue/runtime-dom #7979

Merged
merged 12 commits into from Mar 29, 2023

Conversation

sodatea
Copy link
Member

@sodatea sodatea commented Mar 29, 2023

Based on #7978, please merge that PR first.

Allows augmenting JSX types within declare module '@vue/runtime-dom'.
Fixes use cases such as https://github.com/vuetifyjs/vuetify/blob/29777628ac8839c0548e869d3d350ed9fdcbb149/packages/vuetify/src/shims.d.ts#L18-L25

Drops vue/jsx-runtime/dom.
Because imported (and re-exported) types cannot be augmented, so having the types in 2 different places would certainly break something. So I give up on moving the DOM types to vue/jsx-runtime.
They don't pollute the global namespace anyway.

Otherwise TypeScript will raise TS2322 errors. I have no idea why and
how does the fix work. But it does.

Even importing the `ReservedProps` and `NativeElements` types from
`jsx-runtime` instead of declaring them in the module would fail the
tests. I have no idea why, either.

The failing tests are at https://github.com/vuejs/ecosystem-ci/actions/runs/4538928668/jobs/7998297656#step:7:3
Fixes use cases such as https://github.com/vuetifyjs/vuetify/blob/29777628ac8839c0548e869d3d350ed9fdcbb149/packages/vuetify/src/shims.d.ts#L18-L25

Adds a new `./jsx.d.ts` entry in `@vue/runtime-dom` to avoid duplication
in `vue`. It should be removed in 3.4 when we stop registering global
JSX namespace by default.
@sodatea

This comment was marked as outdated.

@vue-bot

This comment was marked as outdated.

@sodatea

This comment was marked as outdated.

@vue-bot

This comment was marked as outdated.

@sodatea sodatea marked this pull request as draft March 29, 2023 06:32
Drops `vue/jsx-runtime/dom`.
The previous commits didn't work because imported (and re-exported)
types cannot be augmented.
So I give up on the idea of moving the DOM types to `vue/jsx-runtime`.
They don't pollute the global namespace anyway.

There's still one caveat that I have to import the
`IntrinsicElementAttributes` type from
`@vue/runtime-dom/dist/runtime-dom` rather than using the package name
as entry. Seems because in this monorepo, TypeScript always picks up
the source file (`index.ts`) over the built file
(`dist/runtime-dom.d.ts` as specified in the `types` field in
`package.json`)
@sodatea
Copy link
Member Author

sodatea commented Mar 29, 2023

/ecosystem-ci run vuetify

@vue-bot
Copy link

vue-bot commented Mar 29, 2023

📝 Ran ecosystem CI: Open

suite result
vuetify ✅ success

VNode,
VNodeRef,
IntrinsicElementAttributes
} from '@vue/runtime-dom/dist/runtime-dom'
Copy link
Member

Choose a reason for hiding this comment

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

this should be just @vue/runtime-dom right?

@sodatea sodatea changed the title fix: allow augmenting JSX types within declare module '@vue/runtime-dom' fix: move JSX DOM types back to @vue/runtime-dom Mar 29, 2023
VNode,
VNodeRef,
IntrinsicElementAttributes
} from '@vue/runtime-dom/dist/runtime-dom'
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@vue-bot

This comment was marked as outdated.

@sodatea sodatea marked this pull request as ready for review March 29, 2023 09:27
@yyx990803
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link

vue-bot commented Mar 29, 2023

📝 Ran ecosystem CI: Open

suite result
naive-ui ❌ failure
nuxt ❌ failure
pinia ✅ success
router ✅ success
test-utils ❌ failure
vant ❌ failure
vite-plugin-vue ⏹️ cancelled
vitepress ✅ success
vue-macros ❌ failure
vuetify ❌ failure
vueuse ✅ success

@yyx990803
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link

vue-bot commented Mar 29, 2023

📝 Ran ecosystem CI: Open

suite result
naive-ui ❌ failure
nuxt ❌ failure
pinia ✅ success
router ✅ success
test-utils ❌ failure
vant ❌ failure
vite-plugin-vue ❌ failure
vitepress ✅ success
vue-macros ❌ failure
vuetify ✅ success
vueuse ✅ success

@yyx990803 yyx990803 merged commit ffe679c into vuejs:main Mar 29, 2023
13 checks passed
IAmSSH pushed a commit to IAmSSH/core that referenced this pull request May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants