-
Notifications
You must be signed in to change notification settings - Fork 342
feat!: move package to ESM only #1078
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for v-tooltip ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Hello @userquin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request migrates the floating-vue
package to be an ESM-only module. This involves removing CJS artifacts, and introducing subpackage exports for directives, unimport presets, components, and utils. A patching script is added to handle post-build adjustments, including removing static CSS imports from index.d.ts
, adding .d.mts
variants, and fixing .mjs
imports in declaration files. The demo application is also updated to reflect these changes, including adding unplugin-auto-import
to showcase the new unimport presets.
Highlights
- ESM-only Migration: The package is now ESM-only, with all CJS-related code removed.
- Subpackage Exports: New subpackage exports are added for directives, unimport presets, components, and utils.
- Build Patching: A script is introduced to patch the build, handling tasks like CSS import removal and
.d.mts
file generation. - Demo Updates: The demo application is updated to use the new ESM modules and
unplugin-auto-import
.
Changelog
Click here to see the changelog
- demo/package.json
- Added
@tsconfig/node22
and@vue/tsconfig
as dev dependencies. - Added
unplugin-auto-import
andunimport
as dev dependencies. - Added
vue-tsc
andtypescript
as dev dependencies.
- Added
- demo/src/views/directive/VTooltipDemo1.vue
- Added a
<script lang="ts">
block to the component.
- Added a
- demo/tsconfig.app.json
- Added
composite
andtsBuildInfoFile
compiler options. - Added
baseUrl
andpaths
compiler options to mapfloating-vue
andfloating-vue/directives
to their source files.
- Added
- demo/tsconfig.json
- Added references to
tsconfig.node.json
andtsconfig.app.json
.
- Added references to
- demo/tsconfig.node.json
- Added
composite
andtsBuildInfoFile
compiler options. - Set
module
toESNext
andmoduleResolution
toBundler
. - Added
paths
compiler options to mapfloating-vue
andfloating-vue/directives
to their source files. - Set
noEmit
totrue
.
- Added
- demo/vite.config.ts
- Imported
AutoImport
fromunplugin-auto-import/vite
andFloatingVueDirectives
fromfloating-vue/unimport-presets
. - Added resolve aliases for
floating-vue
modules. - Added
VERSION
define. - Added
AutoImport
plugin withFloatingVueDirectives
.
- Imported
- package.json
- Moved
floating-vue
from devDependencies to dependencies. - Added
@types/node
as a dev dependency. - Added
unimport
andunplugin-vue-components
as dev dependencies.
- Moved
- packages/floating-vue/components.d.mts
- Created a new file to export component definitions with
.mjs
extensions.
- Created a new file to export component definitions with
- packages/floating-vue/components.d.ts
- Created a new file to export component definitions.
- packages/floating-vue/components.js
- Created a new file to export components with
.js
extensions.
- Created a new file to export components with
- packages/floating-vue/components.mjs
- Created a new file to export components with
.mjs
extensions.
- Created a new file to export components with
- packages/floating-vue/directives.d.ts
- Created a new file to export directive definitions.
- packages/floating-vue/package.json
- Updated
main
,module
,types
, andexports
fields to reflect ESM-only structure. - Added new exports for components, directives, utils, and unimport-presets.
- Added new files to the
files
array. - Added
unimport
andunplugin-vue-components
as peer dependencies. - Added
unimport
andunplugin-vue-components
topeerDependenciesMeta
.
- Updated
- packages/floating-vue/scripts/patch-build.mjs
- Created a new file containing a script to patch the build output, including removing CSS imports and creating
.d.mts
files.
- Created a new file containing a script to patch the build output, including removing CSS imports and creating
- packages/floating-vue/src/components.ts
- Created a new file to export components.
- packages/floating-vue/src/components/PopperWrapper.vue
- Updated import path for
Placement
type from../util/popper.js
to../utils
.
- Updated import path for
- packages/floating-vue/src/directives.ts
- Created a new file to export directives.
- packages/floating-vue/src/index.ts
- Replaced exports from
./util/events
and./util/popper
with exports from./utils
.
- Replaced exports from
- packages/floating-vue/src/node/types.ts
- Created a new file to define types for directives and unimport presets.
- packages/floating-vue/src/unimport-presets.ts
- Created a new file to define unimport presets for directives.
- packages/floating-vue/src/utils.ts
- Created a new file to export utility functions.
- packages/floating-vue/tsconfig.json
- Excluded
src/node/**
from compilation.
- Excluded
- packages/floating-vue/unimport-presets.d.ts
- Created a new file to export unimport presets definitions.
- packages/floating-vue/utils.d.ts
- Created a new file to export utils definitions.
- packages/floating-vue/vite.config.ts
- Updated
lib.entry
to include multiple entry points. - Added
unimport
andfloating-vue/directives
torollupOptions.external
.
- Updated
- pnpm-lock.yaml
- Updated dependencies and devDependencies to reflect the changes in
package.json
files.
- Updated dependencies and devDependencies to reflect the changes in
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
To ESM we stride,
CJS fades into the night,
Modern modules rise.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request migrates the floating-vue
package to ESM-only, which is a significant change. It also introduces new subpackage exports and build script modifications. The changes seem well-structured, but let's examine a few areas for potential improvements and clarifications.
Summary of Findings
- Missing JSDoc comments: Several functions in
packages/floating-vue/scripts/patch-build.mjs
lack JSDoc comments, which could hinder maintainability and understanding of the code. - Inconsistent import patching: The
patchFileImports
andpatchFile
functions inpackages/floating-vue/scripts/patch-build.mjs
appear to perform similar tasks but use different approaches. Consider consolidating them for better consistency. - Hardcoded paths in vite.config.ts: The
resolve
function indemo/vite.config.ts
uses hardcoded paths, which might not be portable across different environments. Consider using relative paths or environment variables.
Merge Readiness
The pull request introduces significant changes and requires careful review. While the overall structure seems sound, addressing the identified issues and ensuring thorough testing are crucial before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the high severity issues should be addressed before merging.
/gemini review |
I still need to do some more checks and change the build to use only ESM format in vite rollup options, some steps in the patch script won't be required. I'm going to check also using |
) | ||
}) | ||
|
||
nuxt.hook('components:extend', async (registry) => { |
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.
Remove async
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 guess we don't need the plugin using Nuxt auto-import stuff: just register the plugin if imports is disabled.
We should add a new option to the module to use auto-import or global registration.
|
||
nuxt.hook('components:extend', async (registry) => { | ||
const c = prepareFloatingVueComponents(options.components) | ||
for (const component of c) { |
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.
Move prepare to the for loop
"types": ["node"], | ||
"paths": { | ||
"floating-vue/directives": ["../packages/floating-vue/src/directives.ts"], | ||
"floating-vue": ["../packages/floating-vue/src/index.ts"] |
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.
we need to add tsc-alias
as dev dependency, unbuild
or tsdown
will resolve these entries, vite requires resolve.alias
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.
tsdown
doesn't support stubbing yet...
components?: FloatingVueComponentsOptions | ||
} | ||
|
||
const _default: ReturnType<typeof defineNuxtModule> |
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.
review this, I guess we should add a nuxt.ts
and generate the module properly...
Could you release a forked version of this and maintain this project? For me, the project doesn't seem to be active maintained, the last release was already years ago, downstream VitePress and VuePress are suffering with some a11y issues and SSR mismatches with some bugs. I was just planning to create a fork on VuePress org (e.g.: vuepress/ecosystem#477) then I saw this, and this could be the hugest refactor among all PRs, so it could be great if you are interested in maintaining a forked edition and cherry pick some existing PRs with new version being released. Looking forward with your reply, if you are not interested, then I will release a fork of this soon with our bugs fixed. |
This PR doesn't include type module, we can do it later.
This PR includes:
unplugin-auto-import
, check the demo playgroundindex.d.ts
d.mts
variants for modules and vue componentsd.mts
files with correct.mjs
imports (static and dynamic imports)Later I will add also a new
unplugin-vue-components-resolvers
subpackage export to allow use directives and components viaunplugin-vue-components
: 2 new resolvers for directives and components.For context, check my unvuetify monorepo.
TODO:
Migration if using imports:
to