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

Add pure annotation comments for React functions + JSX #1564

Merged
merged 4 commits into from
Apr 11, 2021

Conversation

devongovett
Copy link
Contributor

@devongovett devongovett commented Apr 10, 2021

This ports the /*#__PURE__*/ annotation support for JSX and React methods from Babel to SWC. They are added prior to compiled JSX elements and fragments, and to known pure React methods. This allows minifiers like Terser to remove unused expressions which improves bundle size. I originally implemented this in babel/babel#11428 - see there for more context on why this is useful.

I was a little unsure about where to put the add_pure_comment helper function. I started adding it to the Comments type but then realized that there are several implementations and it ended up being duplicated. Is there a better place to put this?

return String::from_utf8(buf).unwrap();
}

fn run_test(input: &str, expected: &str) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find a way to get the default Tester to pass through comments correctly, so ended up duplicating some of the logic here. Is there a better way?

Copy link
Member

Choose a reason for hiding this comment

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

Adding a field to Tester might work.

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, I tried that. There ended up being too many places to change. e.g. Tester only has one comments and source map instance, but really the actual and expected code need separate instances. Plus the DropSpan pass that's included in the tester breaks comments because it removes the spans that they are attached to. It just got too complicated with too many changes IMO so I bailed. Perhaps some of this logic I copied could be made reusable though. Just wondered if you had ideas or other places I didn't know about that did something similar.

Copy link
Member

Choose a reason for hiding this comment

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

I see.

I used integration test for such tests, but it takes long time to compile.

DropSpan issue can be workarounded by using eq_ignore_span, which is added much later than DropSpan, but it may break lots of tests.

So your approach seems reasonable to me.

Thanks!

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Need some changes, but mostly LGTM. Thanks!

return String::from_utf8(buf).unwrap();
}

fn run_test(input: &str, expected: &str) {
Copy link
Member

Choose a reason for hiding this comment

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

Adding a field to Tester might work.

@@ -37,6 +37,8 @@ pub trait Comments {
fn has_trailing(&self, pos: BytePos) -> bool;
fn move_trailing(&self, from: BytePos, to: BytePos);
fn take_trailing(&self, pos: BytePos) -> Option<Vec<Comment>>;

fn add_pure_comment(&self, pos: BytePos);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a default implementation using take_leading and add_leading?
In this way the pr would be non-breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I'm still a Rust noob and didn't know this was possible. Thanks!

@kdy1 kdy1 added this to the v1.2.52 milestone Apr 10, 2021
@kdy1 kdy1 merged commit 8f5daa3 into swc-project:master Apr 11, 2021
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants