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

feat: Vue 3 support #272

Merged
merged 1 commit into from Oct 16, 2023
Merged

feat: Vue 3 support #272

merged 1 commit into from Oct 16, 2023

Conversation

zernonia
Copy link
Contributor

@zernonia zernonia commented Sep 8, 2023

Update (Sept 22, 2023):

📢 Looks like there's a lot of interest in the Vue integration within the community, so we've released an alpha version 1.3.0-alpha.3 for everyone to test . Any feedback would be greatly appreciated!

https://www.npmjs.com/package/@unovis/vue

@zernonia zernonia marked this pull request as draft September 8, 2023 04:39
@zernonia zernonia marked this pull request as ready for review September 8, 2023 05:18
@zernonia
Copy link
Contributor Author

zernonia commented Sep 8, 2023

@rokotyan here's the PR for #152 ! Let me know anything I should update ya!

Note:

  • Essentially I've updated the autogen/component.ts to fit Vue, and containers component manually.
  • running npm run build will compile using Vite. (reason: rollup-plugin-vue wasn't maintained, and using vite-plugin-vue works perfectly).
  • running npm run check would run vue-tsc to check for ts error.

@rokotyan
Copy link
Contributor

rokotyan commented Sep 8, 2023

@zernonia Thanks for your contribution of the Vue support, that's a major milestone for Unovis!

We'll review the PR in detail next week. In the meantime, here's a few comments:

  1. I was not able to build the library. Getting the following error: image
    Do you have any ideas?

  2. Let's make the vue package a part of the NPM workspace in the top-level package.json:

image
  1. We need to update the README.md file of the package. We also need to update the documentation with Vue snippets and the website examples (I'm currently thinking what would be the easiest way to do this).

  2. We're moving shared code to a separate package: Refactoring: New package for shared code #271. Once we merge it, will you be able to update the Vue package accordingly?

Thanks again for the great work!

@rokotyan
Copy link
Contributor

@zernonia
The #271 pull request has been merged.

In the meantime, can you please submit the CLA. We won't be able to merge the PR without it https://github.com/f5/unovis/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla

@zernonia
Copy link
Contributor Author

@rokotyan do have a look ya! Updated with using the shared package.
I afraid I missed out something important

@productdevbook
Copy link

Nice work @zernonia, have you tested Nuxt 3 ? Let's even add the Nuxt 3 module to it after the pr merger.

@rokotyan
Copy link
Contributor

rokotyan commented Sep 13, 2023

Thanks for the update @zernonia

📢 Looks like there's a lot of interest in the Vue integration within the community, so I've released an alpha version 1.3.0-alpha.0 for everyone to test . Any feedback would be greatly appreciated!

UPDATE: use 1.3.0-alpha.2

@zernonia Can you please create a simple example on StackBlitz for @unovis/vue? I'm sure that will help others to get started.

cc @Atinux @messenjer @Hebilicious @mkarras @sadeghbarati @nirgn975 @DamianGlowala @claytonfaria @anegelya @KABBOUCHI

@rokotyan
Copy link
Contributor

rokotyan commented Sep 13, 2023

@zernonia I've noticed one little but important detail in the Vue package build.

All of our packages are built in a way to allow individual component import (e.g. import { Area } from '@unovis/ts/components/area'). We achieve that with the help of the preserveModules configuration option of Rollup.

Unfortunately, the Vue build doesn't seem to have these individual module builds. When I tried to adopt Vite for Unovis about a year ago, that option was not supported there, so we kept using Rollup. But maybe things have changed since then.

Vue:
image

Other packages:
imageimageimageimage

@Hebilicious
Copy link

Thanks for the update @zernonia

📢 Looks like there's a lot of interest in the Vue integration within the community, so I've released an alpha version 1.3.0-alpha.0 for everyone to test . Any feedback would be greatly appreciated!

@zernonia Can you please create a simple example on StackBlitz for @unovis/vue? I'm sure that will help others to get started.

cc @Atinux @messenjer @Hebilicious @mkarras @sadeghbarati @nirgn975 @DamianGlowala @claytonfaria @anegelya @KABBOUCHI

I think a Vue and a Nuxt one would be a good idea 👍

@zernonia
Copy link
Contributor Author

Hey @rokotyan ! I've updated to enabled preservedModules ya.. It's not as straightforward as using rollup 😅.

Below demonstrates the component directly imported from @unovis/vue, and @unovis/vue/dist/components/area.

image
image

@zernonia
Copy link
Contributor Author

@rokotyan I cant run the package 1.3.0-alpha0 because the package.json for @unovis/vue was wrong. I've push a fix here too. 😁

@rokotyan
Copy link
Contributor

@rokotyan I cant run the package 1.3.0-alpha0 because the package.json for @unovis/vue was wrong. I've push a fix here too. 😁

Thanks! Just published 1.3.0-alpha.1 with those changes

@zernonia
Copy link
Contributor Author

Awesome @rokotyan !!! Here's the Stackblitz demo! 🚀
https://stackblitz.com/edit/vitejs-vite-zb3f5o?file=src%2FApp.vue

@Hebilicious
Copy link

Hebilicious commented Sep 15, 2023

Awesome @rokotyan !!! Here's the Stackblitz demo! 🚀 stackblitz.com/edit/vitejs-vite-zb3f5o?file=src%2FApp.vue

Just porting the example to Nuxt, I'm running into a few commonJS/ESM related errors : https://stackblitz.com/edit/github-spptxs?file=app.vue

require() of ES Module /home/projects/github-spptxs/node_modules/@unovis/ts/index.js 
from /home/projects/github-spptxs/node_modules/@unovis/vue/containers/xy-container/xy-container.cjs not supported. 

Instead change the require of index.js in /home/projects/github-spptxs/node_modules/@unovis/vue/containers/xy-container/xy-container.cjs 
to a dynamic import() which is available in all CommonJS modules.

While we could do some stuff on the Nuxt side to workaround these, is there any chance we could do something in unovis directly ?

@zernonia
Copy link
Contributor Author

zernonia commented Sep 15, 2023

Awesome @rokotyan !!! Here's the Stackblitz demo! 🚀 stackblitz.com/edit/vitejs-vite-zb3f5o?file=src%2FApp.vue

Just porting the example to Nuxt, I'm running into a few commonJS/ESM related errors : https://stackblitz.com/edit/github-spptxs?file=app.vue

require() of ES Module /home/projects/github-spptxs/node_modules/@unovis/ts/index.js 
from /home/projects/github-spptxs/node_modules/@unovis/vue/containers/xy-container/xy-container.cjs not supported. 

Instead change the require of index.js in /home/projects/github-spptxs/node_modules/@unovis/vue/containers/xy-container/xy-container.cjs 
to a dynamic import() which is available in all CommonJS modules.

While we could do some stuff on the Nuxt side to workaround these, is there any chance we could do something in unovis directly ?

Ahh you are right. This is a problem with packages/vue's package.json. Pushing a fix!

@zernonia
Copy link
Contributor Author

Pushed a fix to that esm import issue. Sorry for the trouble @rokotyan 🙈

@rokotyan
Copy link
Contributor

@zernonia @Hebilicious
No problem, just published 1.3.0-alpha.2

@Hebilicious
Copy link

@zernonia great, I updated the Nuxt example : https://stackblitz.com/edit/github-spptxs?file=app.vue

@zernonia
Copy link
Contributor Author

@zernonia great, I updated the Nuxt example : https://stackblitz.com/edit/github-spptxs?file=app.vue

Thanks for making the Nuxt example!! 😁

@zernonia
Copy link
Contributor Author

@rokotyan Upon checking creating gallery example, I've found some minor issue. Pushed the fix as well as those gallery example.

I'll update those path (move to shared) again once you have merge the related PR ya 😁

@rokotyan
Copy link
Contributor

@zernonia This is awesome, thanks for the phenomenal work you do!

We've just merged the shared examples PR, feel free to go ahead and update the paths. Can you please also add a simple demo app to packages/vue so we can test those examples?

@zernonia
Copy link
Contributor Author

@rokotyan I've moved the examples into shared folder, and added a gallery to demo the app locally as well!!.

npm run dev // run gallery demo locally
npm run gallery // build gallery into `dist-demo`

@rokotyan
Copy link
Contributor

@rokotyan I've moved the examples into shared folder, and added a gallery to demo the app locally as well!!.

npm run dev // run gallery demo locally
npm run gallery // build gallery into `dist-demo`

Thanks @zernonia, this is amazing!

When I'm trying to run the app, I get the following error. Should we add de-indent as a dependency?

image

@zernonia
Copy link
Contributor Author

@rokotyan . It's weird that the deps is missing. How yeah adding de-indent and jju as devDep solve the build issue above. 😁 Pushed a fix

@TasoOneAsia TasoOneAsia added the cla-signed Authors have signed the F5 contributor license agreement label Sep 20, 2023
@rokotyan
Copy link
Contributor

Thanks @zernonia, I think we're getting close to the release! There are two main things left:

  1. I noticed that some of the examples don't work. Can you please check what's wrong?
image image image image
  1. We need to update the docs, especially the code snippets. If you feel adventurous, you can give it a try. Or we'll do it ourselves within 1-2 weeks.
image

@zernonia
Copy link
Contributor Author

zernonia commented Sep 21, 2023 via email

@rokotyan
Copy link
Contributor

@zernonia Thanks, I've realized that our postinstall script doesn't build the Vue package. And after manually building it, everything started to work!

That's awesome that you've updated the docs too. One little thing, can you please increase the indentation inside the script tag:
Screen Recording 2023-09-21 at 2 05 03 PM

@reb-dev Can you please also take a look at the PR and share your feedback. I'll be out the first half of the next week, so we can aim for a release it in the first week of October. I think we might want to update the website with some new blocks related to this, since it's a major milestone. I can take care of that once I'm back.

@rokotyan
Copy link
Contributor

rokotyan commented Sep 22, 2023

Could you publish the latest build for @unovis/vue please? It fixes some major bugs in alpha-2.

@zernonia Sure, just published 1.3.0-alpha.3

@zernonia
Copy link
Contributor Author

Tested on https://www.shadcn-vue.com/themes.html, and it works pretty well! Although it's currently missing the props/emits auto-inferred in IDE, but I believe Volar will soon support Generic-typed components 😁

reb-dev
reb-dev previously approved these changes Sep 29, 2023
Copy link
Collaborator

@reb-dev reb-dev left a comment

Choose a reason for hiding this comment

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

Thanks for your incredible work on this @zernonia!

Code looks good and everything works well. @rokotyan Shall we merge it and include it in next week's release?

@rokotyan
Copy link
Contributor

Thanks @reb-dev, I would like to solve the indentation problem before we merge the PR.

Screen Recording 2023-09-21 at 2 05 03 PM

Also we need to add npm run build:vue to postinstall.sh. Everything else looks good to me too.

@messenjer
Copy link

Hi, just for information, in vue component the indentation is 0 in the script part, it's the default rules of eslint and prettier vue, there's just the identation missing in the template part.

@productdevbook
Copy link

Hi, just for information, in vue component the indentation is 0 in the script part, it's the default rules of eslint and prettier vue, there's just the identation missing in the template part.

I agree with that, it's more convenient for us at the vue. 💯

@rokotyan
Copy link
Contributor

rokotyan commented Oct 2, 2023

@messenjer @productdevbook Thanks for the feedback! I didn't realize that Vue has 0 indentation in the script part.

@rokotyan
Copy link
Contributor

rokotyan commented Oct 2, 2023

Applied the final fixes and rebased from main. Planning to take another look tomorrow and then merge.

@rokotyan
Copy link
Contributor

rokotyan commented Oct 3, 2023

@zernonia I was doing the final review today and found that data updates are kind of broken — they only happen once. Here's a few examples from the gallery, there're no errors in the console:

Screen.Recording.2023-10-03.at.3.14.14.PM.web.mp4
Screen.Recording.2023-10-03.at.3.15.06.PM.web.mp4
Screen.Recording.2023-10-03.at.3.13.51.PM.web.mp4

Can you please take a look what's going on?

@zernonia
Copy link
Contributor Author

zernonia commented Oct 4, 2023

@rokotyan I've push a fix for the issue above yaa.. It should be all good now 😁

@rokotyan
Copy link
Contributor

rokotyan commented Oct 4, 2023

Thanks @zernonia , impressed with your work as usual!

Just found another little things: when data is provided to an individual component and it updates, the component doesn't react to that. So the following configuration doesn't seem to be working properly:

  <VisXYContainer ... >
    <VisLine :data="data" ... />
    ...

At the same time when data is provided to the container, it works:

  <VisXYContainer :data="data" ... >
    <VisLine ... />
    ...

Just pushed a test commit demonstrating the problem: 4f9111e

@zernonia
Copy link
Contributor Author

zernonia commented Oct 5, 2023

Ahh yes @rokotyan .. my bad for that.. I've added a watcher (missed that out previously) and it fixed it 😁
Also, I've pushed some performance optimization 😁

rokotyan
rokotyan previously approved these changes Oct 5, 2023
@rokotyan
Copy link
Contributor

@zernonia I was finally going to merge the PR but after the latest rebase getting this error: image

Do you maybe have a clue of what might be wrong?

@zernonia
Copy link
Contributor Author

@rokotyan I'm taking a look!

@zernonia
Copy link
Contributor Author

@rokotyan the issue was related to newly changes Crosshair having WithOptional types, which is causing Vue to not resolve the runtime props correctly.

If the error happens again, you can add the relevant component into the array below until we resolves this issue from Vue.

// Vue 3.3.4 has issue resolving complex Typescript, in this case when the type has `WithOptional`.
// If the build is failing, add the respective component here.
const complexPropComponent = ['Timeline', 'Crosshair']

@rokotyan
Copy link
Contributor

@zernonia I see, good to know. Thanks for the fix!

@rokotyan
Copy link
Contributor

@zernonia I think I've just found another issue (hopefully final). This Sankey example responds only to the first click:

Screen.Recording.2023-10-11.at.2.07.27.PM.web.mp4

I remember you've fixed a similar issue before, maybe they're related?

@zernonia
Copy link
Contributor Author

Hey @rokotyan ! Pushed a simple fix to the example. Not a component issue ya 😁

Co-Authored-By: zernonia <59365435+zernonia@users.noreply.github.com>
@reb-dev reb-dev merged commit eb52eba into f5:main Oct 16, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Authors have signed the F5 contributor license agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants