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

fix named-asset-import plugin to work with export-as syntax #5573

Merged
merged 8 commits into from
Nov 20, 2018

Conversation

NShahri
Copy link
Contributor

@NShahri NShahri commented Oct 26, 2018

The following code doesn't work

export {ReactComponent as LogoIcon} from './logo.svg';

case-2-error-message

But it will work:

import {ReactComponent as LogoIcon} from './logo.svg';
export {LogoIcon};

Sample Project:

https://github.com/NShahri/create-react-app-export-named-assets

Result:

ORIGINAL: When there is no change in the project which is created by CRA
https://github.com/NShahri/create-react-app-export-named-assets/blob/master/test-results/main.e0244443.chunk-original.js

CASE 1: Change to import {ReactComponent as LogoIcon} from './logo.svg'; {LogoIcon};
https://github.com/NShahri/create-react-app-export-named-assets/blob/master/test-results/main.35c51289.chunk-case-1.js

CASE 2: change to export {ReactComponent as LogoIcon} from './logo.svg';
https://github.com/NShahri/create-react-app-export-named-assets/blob/master/test-results/main.0cd4b7da.chunk-case-2.js#L32

CASE 2 with new named-assets-import plugin:
https://github.com/NShahri/create-react-app-export-named-assets/blob/master/test-results/main.92516e27.chunk-case-2-new-named-assets-plugin.js

Note

CASE 1 and CASE 2 with new named-assets-import plugin are identical.

@NShahri
Copy link
Contributor Author

NShahri commented Oct 26, 2018

#4640

@NShahri
Copy link
Contributor Author

NShahri commented Oct 26, 2018

Question:
I am not sure what kind of test I should implement for testing babel plugin, could you please point me to how at the moment this plugin is test.

@NShahri
Copy link
Contributor Author

NShahri commented Oct 26, 2018

Also I added automation tests for this plugin in #5575. I will add tests for export as soon as it is final and has been merged

@Timer Timer requested a review from iansu October 26, 2018 19:09
@iansu
Copy link
Contributor

iansu commented Oct 31, 2018

Thanks for this. Let's get #5575 merged first and then we can tackle this.

@iansu
Copy link
Contributor

iansu commented Oct 31, 2018

Now that #5575 is merged can you please update this branch with master and expand the tests to include the export as syntax?

@Timer Timer added this to the 2.1.x milestone Nov 2, 2018
@Timer Timer requested a review from iansu November 2, 2018 12:48
@NShahri
Copy link
Contributor Author

NShahri commented Nov 8, 2018

@iansu any luck to take a look at this.

@iansu iansu self-assigned this Nov 14, 2018
packages/babel-plugin-named-asset-import/index.js Outdated Show resolved Hide resolved
packages/babel-plugin-named-asset-import/index.js Outdated Show resolved Hide resolved
packages/babel-plugin-named-asset-import/index.js Outdated Show resolved Hide resolved
packages/babel-plugin-named-asset-import/index.js Outdated Show resolved Hide resolved
@iansu
Copy link
Contributor

iansu commented Nov 19, 2018

I finally had a chance to test this locally on the weekend and it works as expected. I requested a few small changes to the PR. Once those are done I think this is ready to go.

@iansu iansu modified the milestones: 2.1.x, 2.1.2 Nov 20, 2018
@iansu iansu merged commit fb465a3 into facebook:master Nov 20, 2018
@iansu
Copy link
Contributor

iansu commented Nov 20, 2018

Thanks!

dardub added a commit to OffBase/create-react-app that referenced this pull request Nov 27, 2018
* upstream/master: (210 commits)
  Support setupTests.ts (facebook#5698)
  Remove unnecessary whitespace in template HTML
  Run prettier on HTML files (facebook#5839)
  Some Grammar fixes (facebook#5858)
  Fix link to page about running tests (facebook#5883)
  fix: make typescriptformatter support 0.5 of fork checker (facebook#5879)
  Always test with the latest stable Node version on Travis (facebook#5546)
  Fix propertyDecorator test
  Upgrade babel deps
  Fix annotated var test
  Fix TypeScript decorator support (facebook#5783)
  fix: add `sideEffects: false` to react-error-overlay (facebook#5451)
  Add allowESModules option to babel-preset-react-app (facebook#5487)
  Make named-asset-import plugin work with export-as syntax (facebook#5573)
  React native repository updated in README.md (facebook#5849)
  extra polyfills must be included manually (facebook#5814)
  Rename 'getting started' link to 'docs' (facebook#5806)
  docs: Simplify installing Storybook with npx (facebook#5788)
  Don't polyfill fetch for Node -- additional files (facebook#5789)
  docs: Change Storybook install documentation (facebook#5779)
  ...
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants