Skip to content

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

Merged
iansu merged 8 commits into
react:masterfrom
NShahri:export-named-assets
Nov 20, 2018
Merged

fix named-asset-import plugin to work with export-as syntax#5573
iansu merged 8 commits into
react:masterfrom
NShahri:export-named-assets

Conversation

@NShahri

@NShahriNShahri commented Oct 26, 2018

Copy link
Copy Markdown
Contributor

The following code doesn't work

export{ReactComponentasLogoIcon}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
Copy Markdown
ContributorAuthor

#4640

@NShahri

Copy link
Copy Markdown
ContributorAuthor

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

NShahri commented Oct 26, 2018

Copy link
Copy Markdown
ContributorAuthor

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 iansuOctober 26, 2018 19:09
@iansu

Copy link
Copy Markdown
Contributor

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

@iansu

Copy link
Copy Markdown
Contributor

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

Comment threadpackages/babel-plugin-named-asset-import/index.js
Comment threadpackages/babel-plugin-named-asset-import/index.js
@TimerTimer added this to the 2.1.x milestone Nov 2, 2018
@Timer
Timer requested a review from iansuNovember 2, 2018 12:48
@NShahri

Copy link
Copy Markdown
ContributorAuthor

@iansu any luck to take a look at this.

@iansuiansu self-assigned this Nov 14, 2018
Comment threadpackages/babel-plugin-named-asset-import/index.js Outdated
Comment threadpackages/babel-plugin-named-asset-import/index.js Outdated
Comment threadpackages/babel-plugin-named-asset-import/index.js Outdated
Comment threadpackages/babel-plugin-named-asset-import/index.js Outdated
@iansu

Copy link
Copy Markdown
Contributor

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.

@iansuiansu modified the milestones: 2.1.x, 2.1.2Nov 20, 2018
@iansu
iansu merged commit fb465a3 into react:masterNov 20, 2018
@iansu

Copy link
Copy Markdown
Contributor

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 (react#5698)
Remove unnecessary whitespace in template HTML
Run prettier on HTML files (react#5839)
Some Grammar fixes (react#5858)
Fix link to page about running tests (react#5883)
fix: make typescriptformatter support 0.5 of fork checker (react#5879)
Always test with the latest stable Node version on Travis (react#5546)
Fix propertyDecorator test
Upgrade babel deps
Fix annotated var test
Fix TypeScript decorator support (react#5783)
fix: add `sideEffects: false` to react-error-overlay (react#5451)
Add allowESModules option to babel-preset-react-app (react#5487)
Make named-asset-import plugin work with export-as syntax (react#5573)
React native repository updated in README.md (react#5849)
extra polyfills must be included manually (react#5814)
Rename 'getting started' link to 'docs' (react#5806)
docs: Simplify installing Storybook with npx (react#5788)
Don't polyfill fetch for Node -- additional files (react#5789)
docs: Change Storybook install documentation (react#5779)
...
@locklockBot locked and limited conversation to collaborators Jan 18, 2019
Sign up for freeto 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

@NShahri@iansu@Timer@facebook-github-bot