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.

Sign up for GitHub

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

Jump to bottom

Feedback requested: Refactor library for Vue v3 support #2

Merged
merged 28 commits into from
Dec 1, 2020
Merged

Feedback requested: Refactor library for Vue v3 support #2

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d464eb2
Refactor library for Vue v3 support
brlodi Nov 16, 2020
0fd5a8e
Oops: actually run Prettier on output files
brlodi Nov 16, 2020
a465c4d
example: show pointer cursor on interactable icons
rektdeckard Nov 18, 2020
79d8583
bin: add <slot> to icons to allow extension of SVGs
rektdeckard Nov 18, 2020
e50ccc3
example: demo <slot> feature by animating and adding elements to icon
rektdeckard Nov 18, 2020
a873c90
Refactor components to Composition API
brlodi Nov 19, 2020
e89a590
Explicitly type icon components (finally!)
brlodi Nov 19, 2020
c82ff93
example: more <slot> demos
rektdeckard Nov 19, 2020
4242f47
meta: bump to v1.0.1
rektdeckard Nov 19, 2020
6550b5b
README: document <slot> behavior and add example
rektdeckard Nov 19, 2020
024aa7d
README: fix example typo
rektdeckard Nov 19, 2020
a0ba386
README: fix example type (again)
rektdeckard Nov 19, 2020
ffa69c1
Make build not error out
brlodi Nov 20, 2020
a30f3a2
example: better demo <slot> mechanism
rektdeckard Nov 22, 2020
c4b808f
lib: bump to v1.0.2
rektdeckard Nov 22, 2020
1ef1c0f
README: switch to kebab-case for examples
rektdeckard Nov 24, 2020
739b691
example: update to use kebab-case
rektdeckard Nov 24, 2020
23ca61c
icons+meta: update assets and components to v1.1.0
rektdeckard Nov 24, 2020
88242aa
icons+meta: fix CloudMoon missing weights and bump to v1.1.1
rektdeckard Nov 25, 2020
f63f18c
icons+meta: update assets and components to v1.1.2
rektdeckard Nov 26, 2020
462242b
Pin Vue at 3.0.2 to avoid bug in 3.0.3
brlodi Nov 30, 2020
4bcfc45
Add prettier to dev dependencies
brlodi Nov 30, 2020
bdc6a18
Restore type check to example app
brlodi Nov 30, 2020
c056172
Prettier run
brlodi Nov 30, 2020
d624b83
Auto-generate library .d.ts file with assemble.js
brlodi Nov 30, 2020
003743c
Prettier and VSCode have a fight over quote style
brlodi Dec 1, 2020
53c10fd
Enable tree-shaking (really it was this easy?!)
brlodi Dec 1, 2020
6c808b6
Merge branch 'master' into vue3
brlodi Dec 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
6 changes: 2 additions & 4 deletions bin/assemble.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ function generateComponents() {
:fill="displayColor"
:transform="displayMirrored"
v-bind="$attrs"
v-on="$listeners"
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Nice to hear that's been changed.

Copy link
Author

Choose a reason for hiding this comment

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

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

>
`;

Expand All @@ -114,15 +113,14 @@ function generateComponents() {
</template>

<script lang="ts">
import Vue from "vue";
import { defineComponent } from "vue";
import {
IconComputed,
IconProps,
PropValidator,
IconContext,
ContextGetter,
} from "@/lib/types";
export default Vue.extend<{}, {}, IconComputed, IconProps>({
export default defineComponent({
name: "Ph${name}",
props: PropValidator,
inject: ContextGetter,
Expand Down
218 changes: 100 additions & 118 deletions example/App.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,73 +332,73 @@
<input name="show" type="checkbox" :checked="showAll" @change="extend" />
<label for="show">Show All</label>
<section v-if="showAll">
<div class="row" v-for="icon in icons" :key="icon.options.name">
<span class="name">{{ icon.options.name }}</span>
<div class="icons" :title="icon.options.name">
<component :is="icon.options.name" />
<div class="row" v-for="icon in icons" :key="icon">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

<span class="name">{{ icon }}</span>
<div class="icons" :title="icon">
<component :is="icon" />
<component
:is="icon.options.name"
:is="icon"
mirrored
color="darkorange"
:size="24"
/>
<component
:is="icon.options.name"
:is="icon"
mirrored
color="darkmagenta"
:size="24"
/>
<component
:is="icon.options.name"
:is="icon"
mirrored
color="royalblue"
:size="24"
/>
<component :is="icon.options.name" :weight="weight" :size="32" />
<component :is="icon" :weight="weight" :size="32" />
<component
:is="icon.options.name"
:is="icon"
:weight="weight"
color="crimson"
:size="32"
/>
<component
:is="icon.options.name"
:is="icon"
:weight="weight"
color="teal"
:size="32"
/>
<component
:is="icon.options.name"
:is="icon"
weight="thin"
:color="color"
:size="48"
/>
<component
:is="icon.options.name"
:is="icon"
weight="light"
:color="color"
:size="48"
/>
<component
:is="icon.options.name"
:is="icon"
weight="regular"
:color="color"
:size="48"
/>
<component
:is="icon.options.name"
:is="icon"
weight="bold"
:color="color"
:size="48"
/>
<component
:is="icon.options.name"
:is="icon"
weight="fill"
:color="color"
:size="48"
/>
<component
:is="icon.options.name"
:is="icon"
weight="duotone"
:color="color"
:size="48"
Expand All @@ -410,29 +410,15 @@
</template>

<script lang="ts">
import Vue from "vue";
import { defineComponent, computed, toRefs, ToRefs, Ref, provide, reactive } from "vue";
import * as Phosphor from "@/entry";
import { ExtendedVue } from "vue/types/vue";
import { IconProps } from "@/lib/types";
import { IconComputed } from "@/lib/types";

type VueIcon = ExtendedVue<Vue, {}, {}, IconComputed, IconProps> & {
options: { name: string };
};

function isIcon(candidate: any): candidate is VueIcon {
return candidate.options && candidate.options.name;
}

const Icon = Object.values(Phosphor).reduce((components, Icon) => {
if (isIcon(Icon)) return { ...components, [Icon.options!!.name]: Icon };
return components;
}, {});

const icons = Object.values(Icon);
// Technically more fragile than the former type guard, but Vue 3 doesn't
const { default: _, ...Icon } = Phosphor;
const allIcons = Object.keys(Icon);
Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


if (process.env.NODE_ENV === "development") {
console.log(`${icons.length} icons`);
console.log(`${allIcons.length} icons`);
}

interface AppData {
Expand All @@ -449,17 +435,18 @@ interface AppData {
open: boolean;
locked: boolean;
muted: boolean;
volume: string;
volume: "high" | "low" | "none" | "mute";
wifi: "high" | "medium" | "low" | "none";
showAll: boolean;
}

export default Vue.extend({
export default defineComponent({
name: "ServeDev",
components: Icon,
data(): AppData {
return {
icons,
setup() {
const toggle = (ref: Ref<boolean>) => ref.value = !ref.value;

const data: ToRefs<AppData> = toRefs(reactive({
icons: allIcons,
weight: "regular",
size: 64,
color: "indianred",
Expand All @@ -475,134 +462,129 @@ export default Vue.extend({
volume: "high",
wifi: "high",
showAll: false
};
},
computed: {
filled() {
const { checked } = this as AppData;
return checked ? "fill" : "regular";
}
},
provide() {
return {
weight: "duotone",
size: 64,
color: "#41B883",
mirrored: false
};
},
methods: {
extend() {
this.showAll = !this.showAll;
},
test() {
console.log(`HI MOM! It's ${new Date().toLocaleTimeString()}`);
},
check() {
this.checked = !this.checked;
},
elapse() {
switch (this.time) {
}));

const filled = computed(() => data.checked.value ? "fill" : "regular");

const extend = () => toggle(data.showAll);
const test = () => console.log(`HI MOM! It's ${new Date().toLocaleTimeString()}`);
const check = () => toggle(data.checked);
const elapse = () => {
switch (data.time.value) {
case "none":
this.time = "high";
data.time.value = "high";
break;
case "high":
this.time = "medium";
data.time.value = "medium";
break;
case "medium":
this.time = "low";
data.time.value = "low";
break;
case "low":
this.time = "none";
data.time.value = "none";
break;
}
},
hide() {
this.visible = !this.visible;
},
discharge() {
switch (this.charge) {
};
const hide = () => toggle(data.visible);
const discharge = () => {
switch (data.charge.value) {
case "full":
this.charge = "high";
data.charge.value = "high";
break;
case "high":
this.charge = "medium";
data.charge.value = "medium";
break;
case "medium":
this.charge = "low";
data.charge.value = "low";
break;
case "low":
this.charge = "empty";
data.charge.value = "empty";
break;
case "empty":
this.charge = "charging";
data.charge.value = "charging";
break;
case "charging":
this.charge = "full";
data.charge.value = "full";
break;
}
},
changeSignal() {
switch (this.signal) {
};
const changeSignal = () => {
switch (data.signal.value) {
case "full":
this.signal = "high";
data.signal.value = "high";
break;
case "high":
this.signal = "medium";
data.signal.value = "medium";
break;
case "medium":
this.signal = "low";
data.signal.value = "low";
break;
case "low":
this.signal = "none";
data.signal.value = "none";
break;
case "none":
this.signal = "full";
data.signal.value = "full";
break;
}
},
changeOpen() {
this.open = !this.open;
},
changeLock() {
this.locked = !this.locked;
},
changeMute() {
this.muted = !this.muted;
},
changeVolume() {
switch (this.volume) {
};
const changeOpen = () => toggle(data.open);
const changeLock = () => toggle(data.locked);
const changeMute = () => toggle(data.muted);
const changeVolume = () => {
switch (data.volume.value) {
case "high":
this.volume = "low";
data.volume.value = "low";
break;
case "low":
this.volume = "none";
data.volume.value = "none";
break;
case "none":
this.volume = "mute";
data.volume.value = "mute";
break;
case "mute":
this.volume = "high";
data.volume.value = "high";
break;
}
},
changeWifi() {
switch (this.wifi) {
};
const changeWifi = () => {
switch (data.wifi.value) {
case "high":
this.wifi = "medium";
data.wifi.value = "medium";
break;
case "medium":
this.wifi = "low";
data.wifi.value = "low";
break;
case "low":
this.wifi = "none";
data.wifi.value = "none";
break;
case "none":
this.wifi = "high";
data.wifi.value = "high";
break;
}
}
}
};

provide('weight', "duotone");
provide('size', 64);
provide('color', "#41B883");
provide('mirrored', false);

return {
...data,
filled,
extend,
test,
check,
elapse,
hide,
discharge,
changeSignal,
changeOpen,
changeLock,
changeMute,
changeVolume,
changeWifi,
};
},
});
</script>

Expand Down
11 changes: 5 additions & 6 deletions example/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import Vue, { VNode } from "vue";
import { createApp } from "vue";
import PhosphorVue from '@/entry';
import Dev from "./App.vue";

Vue.config.productionTip = false;

new Vue({
render: (h): VNode => h(Dev),
}).$mount("#app");
const app = createApp(Dev);
app.use(PhosphorVue);
app.mount("#app");
Loading