search

airbnb / babel-plugin-inline-react-svg

A babel plugin that optimizes and inlines SVGs for your React Components.
MIT License
474 stars 92 forks source link

Fix crash when export declaration has no local property #93

Closed tremby closed 3 years ago

tremby commented 3 years ago

Closes #92

lencioni commented 3 years ago

Thanks for the PR! Can you please add a regression test?

tremby commented 3 years ago

I have no idea about the internals of Babel nor this plugin so I'm not sure I'm well placed to do that. This is just a hack that works for me.

Having said that, if you confirm for me that this is a reasonable fix, and that a correct test would pass in a structure without the "local" property and expect the function to exit without calling applyPlugin, I can add a the test.

lencioni commented 3 years ago

In your issue, your AST showed it was encountering an ExportNamespaceSpecifier. This happens when you have code like this:

export * as foo from 'somewhere';

The ExportNamespaceSpecifier is the "as foo" part.

This code appears to be looking for a pattern like this:

export { default as bar } from 'somewhere';

Where the default export from 'somewhere' is being re-exported as the named specifier bar.

You can see this in this AST explorer here: https://astexplorer.net/#/gist/94c2a82f4d113bd01e39f4078a738f7d/adffef78643a0efd2bf9c1c59ef05456ca3838ab

I believe the fix you have here will prevent the plugin from crashing on the ExportNamedSpecifier in my first example, which is an improvement. A test should be added for this case, that would have failed without your fix.

As for whether we want to do something to handle that in a better way, I'm not really sure! I think probably. It would be great if you could dig into this a little more.

tremby commented 3 years ago

I've added a test which fails before the patch and passes after.

I did start to have a look deeper but I don't even know what the function is meant to do. It's not at all clear to me for example why it's currently only running applyPlugin in this case if .local.name is equal to default.

lencioni commented 3 years ago

Thanks!

lencioni commented 3 years ago

v1.1.2 has been published with this fix!