refactor upgradeConfig.js #50
Conversation
__tests__/upgradeConfig.test.js
Outdated
| @@ -71,3 +71,14 @@ test('does not another flow preset if already present and hasFlow option passed' | |||
| expect(upgradeConfig(config, { hasFlow: true })).toMatchSnapshot(); | |||
| }); | |||
|
|
|||
| test('does not another flow preset if already present in an array and hasFlow option passed', () => { | |||
There was a problem hiding this comment.
noticed this mid-refactor, not sure if this is a case worth handling -- but i figured it couldn't hurt to be safe
There was a problem hiding this comment.
minor grammar fix:
- does not another
+ does not add another
src/upgradeConfig.js
Outdated
|
|
||
| function transformPreset(preset) { | ||
| let presetName = Array.isArray(preset) ? preset[0] : preset; | ||
| const presetOptions = Array.isArray(preset) ? preset[1] : {}; |
There was a problem hiding this comment.
normalizing presets to be "__Name" and "__Options" and then transforming them back down to a string (if options is empty) at the end allows more code to be shared preset/plugin formats
might seem a bit unnecessary right now, but it also simplifies operations on options a bit more
src/upgradeConfig.js
Outdated
| } | ||
|
|
||
| module.exports = function upgradeConfig(config, options) { | ||
| config = Object.assign({}, config); | ||
|
|
||
| changePresets(config, options); | ||
| changePlugins(config); | ||
| if (config.presets) { |
There was a problem hiding this comment.
i was thinking it might be clearer to only mutate config within upgradeConfig, but that might just be personal preference
|
dug in more on reducing duplicate code on my last commit (6fcdef5) may have gone too far with it, but curious to see if anyone thinks parts of it make sense to use here |
src/upgradeConfig.js
Outdated
|
|
||
| if (!Array.isArray(presets) && typeof presets === 'string') { | ||
| presets = config.presets = config.presets.split(',').map((preset) => preset.trim()); | ||
| if (name.indexOf(`babel-${configItemType}`) !== 0 && name.indexOf('@babel/') !== 0) { |
There was a problem hiding this comment.
Can't we use .includes instead of .indexOf? Gives a clearer intent imo.
There was a problem hiding this comment.
hmm -- i agree, but i do think we specifically want to check the beginning of the string, and includes would return true if it were anywhere in it (which would probably be a typo on the user's part, but still)
There was a problem hiding this comment.
we could use startsWith though? may need a polyfill depending on versions of node we're trying to support?
There was a problem hiding this comment.
ah, looks like that should be supported even in node 4. let's use startsWith here
src/upgradeConfig.js
Outdated
| Object.keys(config.env).forEach((env) => { | ||
| changePresets(config.env[env]); | ||
| changePlugins(config.env[env]); | ||
| Object.keys(config.env).forEach(env => { |
There was a problem hiding this comment.
Object.values is what you'd want to use here.
There was a problem hiding this comment.
heh, great point. that totally didn't occur to me while moving this code around 😆
__tests__/upgradeConfig.test.js
Outdated
| @@ -71,3 +71,14 @@ test('does not another flow preset if already present and hasFlow option passed' | |||
| expect(upgradeConfig(config, { hasFlow: true })).toMatchSnapshot(); | |||
| }); | |||
|
|
|||
| test('does not another flow preset if already present in an array and hasFlow option passed', () => { | |||
There was a problem hiding this comment.
minor grammar fix:
- does not another
+ does not add another
| changePresets(config.env[env]); | ||
| changePlugins(config.env[env]); | ||
| }); | ||
| Object.values(config.env).forEach(envConfig => upgradeConfigForEnv(envConfig, options)); |
There was a problem hiding this comment.
If we're supporting node 4 we'll need to polyfill this (or just go back to Object.keys)?
There was a problem hiding this comment.
good point -- i forgot that node 4 doesn't support this. should we be concerned that CI for node 4 isn't failing?
There was a problem hiding this comment.
now that i look back at the tests, env tests could probably be significantly more extensive. this seems like something that should be caught by travis
There was a problem hiding this comment.
I don't think anyone will disagree that we need more tests :)
There was a problem hiding this comment.
noticed the linked PR does add preset-env with target as node 4 as well as adds @babel/polyfill to bin/babel-upgrade, so we should be ok to use Object.values here? confirmed that it works running in node 4 for me locally
src/packageData.js
Outdated
| module.exports = { | ||
| packages, | ||
| presets, | ||
| plugins, | ||
| latestPackages, | ||
| packagesWarnAboutEnv |
There was a problem hiding this comment.
hm not sure what this is, I suspect I pushed it up by accident
There was a problem hiding this comment.
ah yeah i had started looking at the Log when replacing out preset-es2015,16,17,latest as FYI mentioned in the readme at some point and then accidentally added these changes to my commit
|
fwiw most of this code was just waiting on some more time for the tool to bake a bit, and I wouldn't have said it was high on the list of things that needed immediate cleanup. (@hzoo and I had even discussed going to an internal set of plugins that would get passed config/deps down a chain to modify, but said we'd wait and see, and keep it simple if anything) That being said, changes look good. Thoughts @hzoo? |
|
@existentialism that makes sense -- I completely understand if it feels too early to merge in something like this out of curiosity, what parts of the existing code would you prioritize higher for immediate cleanup? |
refactored this file to be (in my opinion) a bit easier to follow
not attached to this approach, very open to other suggestions too!!