feat(icons-vue): add vue 3 support #7545
Conversation
|
✔️ Deploy preview for carbon-elements ready! 🔨 Explore the source changes: 6e7957b 🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-elements/deploys/5ffe22674b4c7b00081b6399 😎 Browse the preview: https://deploy-preview-7545--carbon-elements.netlify.app |
|
✔️ Deploy preview for carbon-components-react ready! 🔨 Explore the source changes: 6e7957b 🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-components-react/deploys/5ffe22673d5cca00070cd4bf 😎 Browse the preview: https://deploy-preview-7545--carbon-components-react.netlify.app |
|
Deploy preview for carbon-elements ready! Built with commit 2885c3c |
|
Looking great @lee-chase! Let me push up a quick fix for the rollup bundle and then you should be good to go 😄 |
|
✔️ Deploy preview for carbon-components-react ready! 🔨 Explore the source changes: 5508688 🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-components-react/deploys/5ffe2272d22ca5000700c7bb 😎 Browse the preview: https://deploy-preview-7545--carbon-components-react.netlify.app |
|
Deploy preview for carbon-components-react ready! Built without sensitive environment variables with commit 2885c3c https://deploy-preview-7545--carbon-components-react.netlify.app |
|
@lee-chase trying to diagnose the ENOMEM that we're seeing now, not sure why this would cause out-of-memory exceptions but will see what we can do 🤔 |
|
Not sure @josh. I was considering a slight refactor to the following, but will hold off while you investigate. /**
* Generate an icon component, which in our case is the string representation
* of the component, from a given moduleName and icon descriptor.
* @param {string} moduleName
* @param {object} descriptor
* @returns {object}
*/
function createIconComponent(moduleName, descriptor) {
const { attrs, content } = descriptor;
const attrsAsString = Object.keys(attrs)
.map((attr) => `${attr}: "${attrs[attr]}"`)
.join(',');
const source = `${BANNER}
import { h } from 'vue';
import { getAttributes } from '@carbon/icon-helpers';
const common = {
name: '${moduleName}',
// We use title as the prop name as it is not a valid attribute for an SVG
// HTML element
props: ['title'],
};
const getSvgAttrs = (title, componentAttrs) => {
return getAttributes({
${attrsAsString},
preserveAspectRatio: 'xMidYMid meet',
xmlns: 'http://www.w3.org/2000/svg',
// Special case here, we need to coordinate that we are using title,
// potentially, to get the right focus attributes
title,
...componentAttrs
});
};
const component = h ? {
// Vue 3 component
...common,
setup(props, { attrs }) {
return () => h(
'svg',
getSvgAttrs(props.title, attrs),
[
props.title && h('title', props.title),
${content.map(convertToVue(3)).join(', ')},
]
);
},
} : {
// Vue 2 component
...common,
functional: true,
render(createElement, context) {
const { children, data, listeners, props } = context;
const attrs = getSvgAttrs(props.title, data.attrs);
const svgData = {
attrs,
on: listeners,
};
if (data.staticClass) {
svgData.class = {
[data.staticClass]: true,
};
}
if (data.class) {
svgData.class = svgData.class || {}; // may be no static class
svgData.class[data.class] = true;
}
// remove style set by getAttributes
delete svgData.attrs.style;
// combine incoming staticStyle, style with default willChange
svgData.style = { ...data.staticStyle, ...data.style };
return createElement('svg', svgData, [
props.title && createElement('title', null, props.title),
${content.map(convertToVue(2)).join(', ')},
children,
]);
},
};
export default component;
`;
return source;
}
/**
* Returns a function that will create a version appropriate Vue source string from a node
* @param {number} version
* @returns {Function}
*/
function convertToVue(version) {
return (node) => {
const { elem, attrs } = node;
return version < 3
? `createElement('${elem}', { attrs: ${JSON.stringify(attrs)} })`
: `h('${elem}', ${JSON.stringify(attrs)})`;
};
} |
|
@lee-chase spent way too long trying to make this work, just seems like no matter what the size of each file * number of files ends up with too many objects being allocated on the heap 😞 I think to move forward, we should see how we could mirror the changes to
|
You are still generating @carbon/icons-react is the intent to drop that? |
|
@lee-chase my comment might have been unclear, there is no intent to drop any packages or change generation logic. The hope was to share the strategy that is being used in // Icon base component
function Icon({ children, ...rest }) {
return /* ... */
}
// Specific icon module / component
function Add16(props) {
return <Icon {...props}><path d="..." /></Icon>;
}This was brought up as one way to potentially reduce the file footprint of each icon module (since a good chunk of duplicate logic can live in the base component). This is needed since the build is experiencing ENOMEM errors from rollup trying to compile all of the modules at once for icons-vue |
|
Interestingly, this builds fine on Node 14, but fails on Node 12 whatever I do to the heap. So I think there is an issue in the Node 12 stream writing that we are hitting here. Nevertheless, I think it's worth refactoring the files along the lines @joshblack mentioned to reduce their overhead, because that brings several benefits and will make things neater and better performance when lots of icons are being used. A base class probably isn't quite so good a fit in Vue 3 as it is in React, but let's push the function into some utility functions and bundle them with the icons -- specialisation by composition rather than inheritance if I remember my CompSci correctly :-). @lee-chase and I have done a first pass, and I'll just polish that and check it works and update the PR including merging master package changes too. |
|
More info in case it's helpful, all the builds should be happening on Node v14 LTS IIRC, specifically for CircleCI that should be defined here. And yeah, definitely didn't mean sub-classes/inheritance but using a base component (in the code snippet that was |
|
@lee-chase I've got a refactor ready to commit, but I think you need to give me permission to update your branch. |
|
@lee-chase @joshblack Here's a refactoring that greatly reduces the weight of each individual icon component file, delegating the work into common functions. It builds a lot faster too! I haven't merged the changes to package.jsons from master in here, because that will bring in a gazillion little yarn cache updates that will clutter things up, but I can easily do that once we're happy with this PR in order to make it mergeable. |
|
@emyarod @joshblack resolving conflicts in the yar.lock file gets dull very quickly every time someone else makes a change as I do not have permission to click resolve conflicts in this repo. |
|
@dcwarwick that's awesome! Definitely will check it out this morning, super exciting!! @lee-chase totally understand, I'm not sure if it's a permissions thing or more-so that through the UI GitHub can't arrange it automatically. If it's a permissions thing I can totally look into it, just let me know what access role it falls under and I can see if I can address it 👍 I went ahead and merged the upstream branch and fixed the merge conflicts, I also reverted the changes to CircleCI so we can check out the full build. Let me know if I missed anything! |
Looks good to me @joshblack |
Closes #7310
Updates Vue icon-build-helper to work with Vue 2 and Vue 3.
Changelog
Changed
packages/icon-build-helpers/src/builders/vue.js
Testing / Reviewing
Verified working in a Vue 3 and Vue 2 project, with click handler, static and dynamic classes, static and dynamic styles.