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

Make babel plugin for auto generating displayName for components #722

Merged
merged 6 commits into from
Mar 25, 2022

Conversation

zephraph
Copy link
Contributor

image

The goal of this PR is two-fold:

  • Make it easier to distinguish and find components when debugging. I was tired of clicking around a bunch of anonymous components or trying to figure which Tabs component I was looking at. As you can see above it's easy to tell at a glance which components are ours and where they come from.
  • Provider a more stable reference for component comparisons across HMR. This is something I've struggled with quite a bit because HMR break's a modules referential integrity by swapping how parts of it (the components in this case) with new defintions of it. That means that comparisons on type is unreliable in dev mode which really sucks. With the addition of this plugin we can compare first on displayName which makes that whole process much easier.

I wrote some relatively comprehensive tests for the babel plugin that covers all our main cases. I even added support for generating displayName for our classed components (like the ContentPane above).

I did hit an issue when it came to actually getting the file to run on our components in libs. Right now vite's react plugin is kind of hard coded to never run babel plugins you give it on files outside of vite's configured root. I tried toying around with includes, etc but not dice. This was also reported on the vite project (vitejs/vite-plugin-react#16) and there's a PR to resolve it but that seemed like more than I really wanted to get into at the moment. For now I've re-added patch-package and made a tiny change to get this to work.

@vercel
Copy link

vercel bot commented Mar 24, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/Hk24VuahaLAv6CvA5aRCB3h2eCr6
✅ Preview: https://console-ui-storybook-git-make-babel-plugin-a7a4af-oxidecomputer.vercel.app

@github-actions
Copy link
Contributor

Comment on lines +10 to +11
+ // Switch to cwd so babel plugin will run outside of project root
+ const isProjectFile = !isNodeModules && (id[0] === "\0" || id.startsWith(process.cwd() + "/"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing this without a patch (or it being fixed upstream) would require that we change vite's root. Right now it's pointing to app. I tried setting it to the repo root, but it won't work unless the html file is there (that despite also having to configure where the html file is in the config...). So we'd either have to move the html file to a dir which is already crowded (I'm not a fan) or change the directory structure such that app and lib shared a root folder. It could be src/* with src/app, src/libs, etc. Still not incredibly ideal given the churn it'd cause. Due to that, I just decided this was the easiest mechanism for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't mind moving stuff under a src dir, that's pretty common. I don't really like changing how Vite works. If anything it's a sign that we're currently Doing It Wrong. However, I'm fine merging this and doing the file shuffle later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agreed. I think eventually moving things to src is the right move. Then again, there's an issue upstream that could sort this out so if that happens maybe it's completely unncessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's still probably worth doing. We could move app, types, and libs in there, and we could put the libs top level under src, they don't need to be under a src/libs dir.

/**
* Helper to streamline babel transforms
*/
function transform(strings: TemplateStringsArray) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

cute

Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Cool. I almost never use the inspector in the React devtools, but maybe I'll start.

@zephraph zephraph merged commit 522ac12 into main Mar 25, 2022
@zephraph zephraph deleted the make-babel-plugin-for-display branch March 25, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants