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: component exports #166
fix: component exports #166
Conversation
@farnabaz I can spin up a reproduction if you need to test |
package.json
Outdated
@@ -24,6 +24,14 @@ | |||
"types": "./dist/runtime/index.d.ts", | |||
"import": "./dist/runtime/index.mjs" | |||
}, | |||
"./runtime/components/MDCRenderer": { |
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.
Let's add components
directory in exports
and not just MDCRenderer
.
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.
I don't believe this is possible with the current state of the repo and exported types.
There is no generated runtime/components/index.mjs
so we can't import directly from there. Also, the MDC.vue
component and none of the runtime/components/prose/*.vue
files are generating types in the build.
If we created a runtime/components/index.ts
file that exported all of the components (a barrel file) it would then trigger the proper exports, but could have an impact on tree-shaking; however, the other components like MDC.vue
utilize Nuxt-specific imports, meaning it would actually break using in a non-Nuxt project.
Since only the MDCRenderer.vue
component was "cleaned" of Nuxt imports, exporting anything else as-is would break 😬
Good with this for now, or any ideas?
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.
@farnabaz here's the updated info.
As for the tests, if I was going to write something I'd likely just want to use vitest
along with @vue/test-utils
so that I can ensure nothing from the Nuxt app is required. I'd need to add that package as a devDependency
; any issues there?
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.
Why not just use "./runtime/components/*": "./dist/runtime/components/*",
in package exports? Then we can import like import MDCRenderer from '@nuxtjs/mdc/runtime/components/MDCRenderer.vue';
Vitest and Vue test-utils seems fine for me 👍
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.
@farnabaz I adjusted the exports in d3108ec to this:
"./runtime/components/*": {
"types": "./dist/runtime/components/*.d.ts",
"import": "./dist/runtime/components/*"
},
"./dist/runtime/components/*": {
"types": "./dist/runtime/components/*.d.ts",
"import": "./dist/runtime/components/*"
}
For the types
property, I had to add *.d.ts
in order to properly resolve the component types.
Technically this doesn't allow for importing types from the nested /runtime/components/prose/*
components, but the build
command doesn't generate declaration files for these components anyway, so I think it's fine?
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.
LGTM 👍
We can update those components later to generate ts file
Thanks, @adamdehaven |
When the
MDCRenderer
component export was removed in 9dc5cc3 it actually broke thepackage.json > exports
ability to resolve the component import.The change in this PR allows for the following import:
I updated the docs, but also wondering if it would make sense to have test coverage for a non-Nuxt implementation in a separate PR?