Feedback requested: Refactor library for Vue v3 support

[brlodi]

NOTE: this should not actually be merged into master. Vue v3 and Vue v2 are API-incompatible and support for them can't easily coexist in the same branch/release, but GitHub won't let me submit a PR targeting a new upstream branch.

---

I stumbled upon Phosphor the other day, and being such a lovely little icon set I was quick to drop it into a personal project. Unfortunately my project is written in the recently-released Vue v3, which has several breaking changes from Vue v2, and plugins for older versions of Vue will not work. Rather than opening an issue asking for an update on a project with active commits today (well, one of the other Phosphor repos), I figured I'd dive in and take a stab at the bulk of the porting work.

Wherever possible, I have kept as close to the Vue v2 code as I could, but I was forced to make typing judgement calls (by which I mean "letting the compiler infer the types") in a few places that should probably be double-checked and corrected. The only exception to the hands-off approach is in the example app, which I took the liberty of porting to the Vue v3 composition API since I ended up having to touch most of the LOC in it anyway.

I still need to do some more in-depth testing, but the unscientific method of "the demo site works" has shown positive results so I figured I'd submit this sooner rather than later.

[brlodi]

Pardon the truncated comment here. The gist is that some of the necessary types for the type guard are private in Vue 3, and rather than putting two more hours into sorting the mess out I just opted for simply omitting default from the plugin import and assuming the remaining members are all icon components.

If this were something a user would be exposed to I'd solve the typing problem, but since this is only a demo site I didn't think it too much of a burden to keep in sync with any new exports from entry.ts.

[rektdeckard]

Yes, entry.ts only registers the exported members of lib/index.ts which are all icon components, so your guard is good for now.

[rektdeckard]

Stellar to see a PR like this, thanks for putting in the time! On first glance it seems solid to me, but I have exactly zero experience with Vue3 and little enough with Vue2 to start. That being said, I'll give it a whirl on my end and see what I see.

I'm not too bothered about relaxing the type checks in the example app, but I'd like to keep explicit types in components if it's doable in v3. Personally, types feel a bit clunky in Vue so I feel your decision to forge ahead for now. And yes, entry.ts only registers the members of lib/index.ts, which are all icon components, so your guard is good for now.

If you have ideas for improvement when it comes to making this more idiomatic Vue, please feel free to share. I am very much a React person writing what they think is a passable Vue port 😄.

I've made a vue3 branch in this repo if you want to continue to work from there.

[rektdeckard]

Example the first... I've been told that in Vue you don't pass callbacks down, but it seems like that just means you need to manually make components emit every type of event anyone might want to hook into. This was my attempt at a clean solution. Is there something more idiomatic?

[brlodi]

For more complex components Vue gently recommends explicitly defining and distributing all component props and then turning on strict prop checking for the component, so anything extraneous errors(warns?) at runtime. This sort of use-case where the component is a thin wrapper around another element is one of the example exceptions though, and you’re not breaking any Vue conventions by doing it.

In Vue 3 the way listeners are handled has been streamlined, so all the native listeners are just on $attrs unless you explicitly emit a custom event with the same name.

This actually worked fine in Vue 3 unchanged but throws a deprecation warning.

[rektdeckard]

Nice to hear that's been changed.

[brlodi]

I was pleasantly surprised that the fix for the deprecation notice was just to delete the line and everything kept working as expected!

[rektdeckard]

Do components get properly stringified in this case? If so, neat.

[brlodi]

I changed the assignment to icons to an Object.keys call, so it’s just the string names of the components. No magic here.

The demo app now globally loads the plug-in the same way a consumer app would, using app.use(PhosphorVue) in the entry point, so there wasn’t a need to dynamically register all the component objects anymore and I just dropped the references to them.

[rektdeckard]

Gotcha, missed that the first time around! Does this give us tree-shaking as well, if you were to only app.use(SomeIcon)?

[brlodi]

You couldn't app.use(SomeIcon), because the icons aren't Plugin typed (i.e. they don't have an install function), but you should be able to import individual icon components like import { PhFoo, PhBar, PhBaz } from 'phosphor-vue' and that should theoretically get tree-shaken.

You could do it in the app entry point to globally register them like app.component(PhFoo) if you expect to use it a lot of places, or you could import it in a specific SFC and locally register it there.

[brlodi]

^ that is assuming I configure Rollup properly to build anything at all 😆

[brlodi]

Thanks for taking a look! I’d never written a Vue plugin before so it was a fun opportunity to dive into that side of things.

I'm not too bothered about relaxing the type checks in the example app, but I'd like to keep explicit types in components if it's doable in v3. Personally, types feel a bit clunky in Vue so I feel your decision to forge ahead for now. And yes, entry.ts only registers the members of lib/index.ts, which are all icon components, so your guard is good for now.

It’s definitely my intent to keep the components type-safe and as explicit as possible. The personal project I was working on is actually my first real foray into combining Vue and TS, since as you say it was pretty clunky in Vue v2 and my professional uses of Vue have therefore been plain JS in the past with TS reserved for backend code.

It has definitely improved in Vue v3, I’ve found, but also relies on synthetic types for things like the component object returned by defineComponent. I spent a solid hour trying to manually type a variable to store one, but there seems to be a little bit of prop-mangling magic going on in the background and my head was starting to swim. Things like optional props becoming required members in the but not when I manually declare the type, etc. I’ll take another look at it today.

If I sort that out I can of course reinstate the actual type guard in the demo app too.

If you have ideas for improvement when it comes to making this more idiomatic Vue, please feel free to share. I am very much a React person writing what they think is a passable Vue port 😄.

I won’t say I didn’t notice a little React-ness 😁, but everyone stumbles on Vue from a different background. I came from Ember so I definitely have “wait I have to do that myself?” moments where I just expect to assign something to a magic variable and have it automatically become a loading spinner or whatever.

I've made a vue3 branch in this repo if you want to continue to work from there.

Cool I’ve rebased the PR to that branch.

[brlodi]

After quite a bit of puzzling, and fighting the TS language server to actually show me the full inferred type, I think I have puzzled it out. I opted to type the return value of defineComponent and let the function types be inferred, since personally

const component: PhosphorIcon = defineComponent({
  props: PropValidator,
  setup(props: SetupIconProps) {
    return { ...useDefaultPropsFromContext(props) };
  }
});

seems much more readable and informative than

export default defineComponent<
  typeof PropValidator,
  ToRefs<IconProps>,
  unknown,
  {},
  {},
  ComponentOptionsMixin,
  ComponentOptionsMixin,
  Record<string, any>,
  string
>({
  props: PropValidator,
  setup(props: SetupIconProps) {
    return { ...useDefaultPropsFromContext(props) };
  }
});

and the type of the component is what we need elsewhere anyway.

---

What I haven't quite figured out is the build. I don't know if I just fundamentally misunderstand TypeScript reexports, if I mangled something in the Rollup config, or if VSCode is caching/hiding something from me, but I can't for the life of me get the individual components to be accessible when loading the bundled library. I'd appreciate your help sorting that out if you get the chance, because it's probably something incredibly obvious that I'm missing.

[rektdeckard]

I like it! Just grokking over the refactor now, I'll see if I can troubleshoot the bundler issues. Had the previous commit working like a dream when I tested last night, btw.

[brlodi]

I should probably note that I haven’t pushed any of my build-system changes to this PR, so it definitely won’t build as-is.

[rektdeckard]

Component typings look great, if somewhat inscrutable -- thanks Vue! Not likely to be that useful for the end-user, but I like the security it buys for codegen and anyone wishing to extend. Also liking the streamlined context injection.

As for the build setup, not sure I'll be of much use as I'm not familiar with Vue build tools and used a template to set this up for Vue 2. I did find this though: https://dev.to/shubhadip/vue-3-component-library-270p

[brlodi]

Actually getting it to output JS bundles is easy enough, the issue is in the types.

Some of my confusion was created by the fact VSCode apparently only reads .d.ts files for an import once until you restart the entire app. I've found that's true even if you fully delete the declarations file, let VSCode complain it's gone, and then create the new version, so debugging this is a very tedious process.

Nothing I've tried has managed to generate a single declarations file with all the necessary exports and types listed. In fact, none even have the named exports at all, as though export * from '...' is just passed straight through TSC with no analysis. I had to write manual types for the output bundle, which came down manually replicating the entire set of type declarations into phosphor-vue.d.ts and manually declaring the exported component members. This "works", but seems like the textbook example of non-DRY code, and is the specific thing I'd love help with because clearly this isn't the way and apparently I don't know what I'm doing.

phosphor-vue.d.ts, which includes essentially the entirety of types.ts as declares

import { Plugin, DefineComponent, AllowedComponentProps, ComponentCustomProps, ComponentOptionsMixin, ToRefs, VNodeProps, PropType } from "vue";

declare type PhosphorVuePlugin = Plugin & { installed?: boolean };
declare const PhosphorVue: PhosphorVuePlugin;
export default PhosphorVue;

declare type Weight = "thin" | "light" | "regular" | "bold" | "fill" | "duotone";
declare type Size = string | number;

declare interface IconProps {
  weight: Weight;
  size: Size;
  color: string;
  mirrored: boolean;
}

declare  type SetupIconProps = Readonly<
  Required<Pick<IconProps, "mirrored">> & Partial<Omit<IconProps, "mirrored">>
>;

declare type PropValidatorType = {
    color: StringConstructor;
    size: PropType<Size>;
    weight: PropType<Weight>;
    mirrored: BooleanConstructor;
}

declare type PhosphorIcon = DefineComponent<
  PropValidatorType,
  ToRefs<IconProps>,
  unknown,
  {},
  {},
  ComponentOptionsMixin,
  ComponentOptionsMixin,
  Record<string, any>,
  string,
  VNodeProps & AllowedComponentProps & ComponentCustomProps,
  SetupIconProps,
  Required<Pick<IconProps, "mirrored">>
>;
export const PhActivity: PhosphorIcon;
export const PhAirplane: PhosphorIcon;
export const PhAirplay: PhosphorIcon;
export const PhAlarm: PhosphorIcon;
export const PhAndroidLogo: PhosphorIcon;
       ...

[brlodi]

I haven't abandoned this! It's currently working except for tree-shaking, though I'm still not happy with the solution for outputting types (I'm generating phosphor-vue.d.ts as an extra step in assemble.js). I'll try to get tree-shaking working in the next couple days to really cut down on the library size.

[rektdeckard]

Thank Ben, and apologies I haven't had much of a chance to work look at this either. My solution for tree-shaking in the React lib was to set perserveModules: true option in the Rollup config for esm builds. This creates separate .esm.js and .d.ts build artifacts for each icon, and a root index.esm.js and index.d.ts files that export all of those. Not sure if that's the only way, but it worked for me there.

[brlodi]

No worries, it's a busy time of year. The tip on preserveModules really paid off! Only took a few minutes to get things working. Things seem to be tree-shaking now, at least as well as they can under the Webpack version that vue-cli uses right now (Webpack v4).

I'll get this branch up to speed with master and mark the PR ready momentarily.

[rektdeckard]

Beautiful! Tested in dev and prod builds and everything seems to be in order, tree-shaking included. Merging this now. Will publish as phosphor-vue3 on npm.

Thanks for the efforts, glad to have you here.

[rektdeckard]

Oh, does documentation need to be updated? No longer need to specify components option with Vue3, right? Anything else we're forgetting?

EDIT: nope, nevermind. Still required, we just call app.use(PhosphorVue) in the example app.

[brlodi]

RE: publishing, the loose standard for Vue 2/3 support seems to be publishing the Vue 3 version under a suitable major version bump (where possible the Vue 3 version under 3.x, but obviously that’s dependent on the library’s own versioning matching up), then marking the latest release of that major version with the @next tag. That way users get the latest Vue 2.x compatible versions with ‘phosphor-vue’, and can install the Vue 3.x version with e.g. ‘phosphor-vue@next’

[rektdeckard]

I guess now you mention it I've seen that pattern, but not normally applied to parallel releases/architectures. How would one go about getting a specific version of the vue3 lib in that case, should they need to?

[brlodi]

You'd just specify the version range as normal, hence why it's necessarily accompanied by a major version bump in addition to the tags. I've seen a couple libraries actually bump the major numbers a couple numbers to leave room for an additional Vue 2 compatible major release, but where possible they seem to be sticking to 2.x and 3.x.

For example, Vue itself is set up this way. Running npm install vue is silently expanded to npm install vue@latest and gets you "vue": "^2.6.12" at the moment, while running npm install vue@next gets you "vue": "^3.0.3". You can also just specify those versions manually in package.json like any other package (which I've actually done in this PR¹, because Vue 3.0.3 has an undocumented breaking change), it just makes it convenient to install either version and keep the NPM version semantics.

¹ Come to think of it, that version is wrong; should probably be >=3 <3.0.3. I'll PR a fix in a sec, and I can update the docs while I'm at it.