FormidableLabs / builder-victory-component

Builder archetype for Victory components
MIT License
1 stars 6 forks source link

opt-in lodash features #64

Closed boygirl closed 8 years ago

boygirl commented 8 years ago

cc/ @ryan-roemer or @exogen

I audited victory for usages of lodash, and based on what I found it seems like these opt-ins should be required for LodashModuleReplacementPlugin.These weren't enabled, and nothing seemed to break in development, which gives me pretty low confidence that I understand what is being included with these feature sets. Any insights you have would be greatly appreciated.

ryan-roemer commented 8 years ago

@exogen -- Can you shepherd this?

@boygirl -- Which parts would you like input on? For example, with paths you just need to check if any of our usage of get() or similar things is using a deep (foo.bar.baz) path or just shallow (foo) paths...

boygirl commented 8 years ago

I started looking into this while hunting down this bug https://github.com/FormidableLabs/victory-chart/issues/241

I am using methods like flatten and partialRight which I would expect to fail without the feature sets included in this PR, and they don't fail which tells me I'm missing something big about how this plugin is working.

ryan-roemer commented 8 years ago

@boygirl -- Interesting. Might be nice to post a gist of the diff in the unminified bundle for victory.js itself in this PR so we can look at that here when hashing this out...

nfcampos commented 8 years ago

I'm not entirely sure how the webpack lodash plugin works but this bit in the code https://github.com/lodash/lodash-webpack-plugin/blob/master/src/index.js#L36 seems to suggest that if you import some module directly the features it needs will be turned on eg. if i understood it correctly

import flatten from 'lodash/flatten'
flatten([])

will work because the flatten module is imported directly. might explain why flatten is working even though you didn't whitelist flattening in the options

edit: and since you're using babel-plugin-lodash, even if you imported like this import {flatten} from 'lodash'the babel plugin will have turned that into that direct import above

boygirl commented 8 years ago

After a little more digging, the bug I was seeing was due to iteratee is not a function which would be caused by missing the shorthands feature set.