-
Notifications
You must be signed in to change notification settings - Fork 51
fix: Inject component annotations into HTML elements rather than React components #848
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
base: main
Are you sure you want to change the base?
fix: Inject component annotations into HTML elements rather than React components #848
Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
🤖 This preview updates automatically when you update the PR. |
729d328 to
d498cc1
Compare
32c96e7 to
148b620
Compare
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll let other folks also review, especially also get perspective from ReactNative so that we're on the same page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-l: Should we refactor the tests to use toMatchInlineSnapshot? Feels way easier to me to associate which test corresponds to which snapshot. Also fine to tackle as a follow-up or not at all if I'm missing a reason for keeping it as-is. WDYT?
packages/babel-plugin-component-annotate/test/__snapshots__/test-plugin.test.ts.snap
Outdated
Show resolved
Hide resolved
s1gr1d
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but as discussed offline, we can put this behind a flag to reduce bundle size impact
a03a02a to
615a4ef
Compare
| "@react-navigation", | ||
| ]; | ||
|
|
||
| export const REACT_NATIVE_ELEMENTS: string[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are missing elements here (e.g. Pressable, TouchableOpacity that we are using in our samples). I'm not sure we can include everything in this list tbh. The users may also add custom components in their apps.
I was wondering if we can make this configurable so that we can add more elements on the RN SDK side or even allow for end users to extend.
antonis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we do not pass experimentalInjectIntoHtml: true from the React Native SDK my understanding is that there is no change in behavior and the changes are safe to ship.
We can experiment with turning the feature ON and see the impact on our side. I think the main issue is the missing components before making this behavior default on the RN SDK.
Rather than inject attributes into React components which can cause incompatibilities with props, this PR changes the plugin to instead inject attributes into every HTML element in the component root.
Downsides
Potentially larger bundle sizes because the attributes will get injected into more elements.