-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Named asset import for SVG files #3907
Changes from 1 commit
45d6324
3a1e13f
3d1de03
f172ee3
8cdc51a
1e2d8a5
d27b488
68439ac
f047ac0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
'use strict'; | ||
|
||
const { extname } = require('path'); | ||
|
||
function namedAssetImportPlugin({ types: t }) { | ||
const visited = new WeakSet(); | ||
|
||
return { | ||
visitor: { | ||
ImportDeclaration(path, { opts: { loaderMap } }) { | ||
const sourcePath = path.node.source.value; | ||
const ext = extname(sourcePath).substr(1); | ||
|
||
if (visited.has(path.node) || sourcePath.indexOf('!') !== -1) { | ||
return; | ||
} | ||
|
||
if (loaderMap[ext]) { | ||
const addLoader = loaderMap[ext]; | ||
|
||
path.replaceWithMultiple( | ||
path.node.specifiers.map(specifier => { | ||
const localName = specifier.local.name; | ||
|
||
if (t.isImportDefaultSpecifier(specifier)) { | ||
const newDefaultImport = t.importDeclaration( | ||
[t.importDefaultSpecifier(t.identifier(localName))], | ||
t.stringLiteral(sourcePath) | ||
); | ||
|
||
visited.add(newDefaultImport); | ||
return newDefaultImport; | ||
} | ||
|
||
const newImport = t.importDeclaration( | ||
[ | ||
t.importSpecifier( | ||
t.identifier(localName), | ||
t.identifier(specifier.imported.name) | ||
), | ||
], | ||
t.stringLiteral( | ||
addLoader(sourcePath, specifier.imported.name, localName) | ||
) | ||
); | ||
|
||
visited.add(newImport); | ||
return newImport; | ||
}) | ||
); | ||
} | ||
}, | ||
}, | ||
}; | ||
} | ||
|
||
module.exports = namedAssetImportPlugin; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"name": "babel-plugin-named-asset-import", | ||
"version": "0.1.0", | ||
"description": "Babel plugin for named asset imports in Create React App", | ||
"repository": "facebookincubator/create-react-app", | ||
"license": "MIT", | ||
"bugs": { | ||
"url": "https://github.com/facebookincubator/create-react-app/issues" | ||
}, | ||
"main": "index.js", | ||
"files": [ | ||
"index.js" | ||
], | ||
"dependencies": { | ||
"@babel/core": "7.0.0-beta.38" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,6 +115,14 @@ module.exports = function(api, opts) { | |
regenerator: true, | ||
}, | ||
], | ||
[ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably have an opt-out flag in options. Just like the flow flag we now have. |
||
require('babel-plugin-named-asset-import'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be enabled in the test environment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just about to ask for suggestions on how to fix these tests. 😀 I was trying to find a way to use Jest's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about moving this to webpack configs instead of keeping it in the preset? That would make more sense to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know we talked about that before and I tried it but I couldn't get it to work. For some reason when I move this to the webpack config it strips out functions. In this case the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you push a commit that tries to implement it, even if it doesn’t work? I think we need to figure out what breaks there. Fine to do as another PR against your existing PR if you prefer so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually try rebasing. We don’t write babelrc anymore (afaik) since monorepos landed. So it doesn’t get serialized on eject and thus it may be solved now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I merged the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
{ | ||
loaderMap: { | ||
svg: filename => `-!svg-react-loader!${filename}`, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, maybe this should be in webpack Babel config and not in our preset? Since it’s directly related to webpack specifically. This also would make a peer dependency on the loader unnecessary (it currently would be). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, either works. The only nice thing about keeping it here is that we don't have to duplicate it in the dev and prod webpack configs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can solve it in other ways (e.g. by actually starting to modularizing configs a little bit). Doesn't have to be solved here. |
||
}, | ||
], | ||
isEnvProduction && [ | ||
// Remove PropTypes from production build | ||
require('babel-plugin-transform-react-remove-prop-types').default, | ||
|
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.
Why? Maybe this should be a peer dependency?
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 originally wrote this plugin outside create-react-app so I needed this. I can change it to a peer dependency.