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

Allow direct imports to reduce bundle size. #2893

Merged
merged 1 commit into from May 7, 2017

Conversation

ruiaraujo
Copy link
Contributor

@ruiaraujo ruiaraujo commented May 6, 2017

Webpack 2 does not remove a lot of unused modules from my bundle so direct import remain the best solution to reduce bundle size.

The doc page was heavily inspired by https://github.com/ReactTraining/react-router/blob/v3/docs/guides/MinimizingBundleSize.md

@codecov-io
Copy link

Codecov Report

Merging #2893 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2893   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          68      67    -1     
  Lines        1404    1407    +3     
======================================
+ Hits         1404    1407    +3
Impacted Files Coverage Δ
src/createFieldArray.js 100% <100%> (ø)
src/createReduxForm.js 100% <100%> (ø)
src/createFormValueSelector.js 100% <100%> (ø)
src/createValues.js 100% <100%> (ø)
src/createFields.js 100% <100%> (ø)
src/immutable.js 100% <100%> (ø) ⬆️
src/createField.js 100% <100%> (ø)
src/index.js 100% <100%> (ø) ⬆️
src/createReducer.js 100% <100%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cda2572...f83c9af. Read the comment docs.

@gustavohenke
Copy link
Collaborator

Can you check #2315, please?
Over there guys think the bundle size problem is related to lodash.

BTW, what a huge PR!

@ruiaraujo
Copy link
Contributor Author

For me since I was also using lodash, it is shared so that is not a problem. The problem was the factory index files that prevented direct imports.

@erikras
Copy link
Member

erikras commented May 7, 2017

Wow! That's an impressive PR!

Well done. Thank you for your time.

@erikras erikras merged commit ff213e8 into redux-form:master May 7, 2017
@ruiaraujo ruiaraujo deleted the smallerBundle branch May 8, 2017 08:02

Use `es` if you are using a bundler that can process ES modules like `webpack@2` or `rollup`, otherwise use `lib`.

The public API available in this manner is defined as the set of imports available from the top-level `react-router` module.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Missed a react-router. 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

god dammit. Do you fix it or should I?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next time I 'inspire' myself on the work of others, do cmd+R. 10 year old me would not have done this kind of mistake. 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #2898

@erikras
Copy link
Member

erikras commented May 8, 2017

@ruiaraujo Is there some advantage to doing

import { default as Foo } from './somedir/Foo'

over

import Foo from './somedir/Foo'

??

The latter is less explicit, but much more readable, in my opinion.

@erikras
Copy link
Member

erikras commented May 8, 2017

Oops, nevermind. They are EXPORT statements. There, now we've both made absentminded mistakes on this PR. :-)

erikras added a commit that referenced this pull request May 8, 2017
erikras pushed a commit that referenced this pull request May 8, 2017
* Add PropTypes for the Field components

* Export fieldInputPropTypes, fieldMetaPropTypes and fieldPropTypes

* Merged with changes from #2893
erikras added a commit that referenced this pull request May 8, 2017
erikras pushed a commit that referenced this pull request May 8, 2017
* Exports defaultShouldValidate and defaultShouldAsyncValidate

These default functions are exported for external use - particularly so
that a user can wrap the default functionality with specialised behaviour.

To resolve #2890

* Export default shouldValidates in immutable wrapper

* Allow direct imports to reduce bundle size. (#2893)

* Fix deep equal on maps with different structures (#2892)

* Remove references to react-router from doc page. (#2898)

* Add PropTypes for the Field components (#2862)

* Add PropTypes for the Field components

* Export fieldInputPropTypes, fieldMetaPropTypes and fieldPropTypes

* Merged with changes from #2893
@erikras
Copy link
Member

erikras commented May 8, 2017

Ugh. ☹️ In retrospect, you really should have git mv'd reduxForm.js to createReduxForm.js, and then created a new reduxForm.js, as this PR has wiped all annotation history from the file, since all the complex code now exists in a brand new file. Oh well... live and learn...

(I probably would not have thought to do this either. I'm just only realizing this in retrospect.)

@ruiaraujo
Copy link
Contributor Author

ruiaraujo commented May 8, 2017 via email

@erikras
Copy link
Member

erikras commented May 8, 2017

Hmm... So it is. I was under the impression that it would preserve the history of the file. I swear I've seen it do that before. Perhaps it only works if you don't immediately add another file with the old name...? Oh well.

Nevermind then. 😄

@gajus
Copy link

gajus commented May 8, 2017

Is there a timeline for this getting released. :-) ?

}
```

##Caveat (Action Creators)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the documentation, it is not clear whether this step is automated by babel-plugin-transform-imports or do I still need to use react-router/es/actions when using the plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to be more explicit about the problem​ then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL, even the follow-up question didn't notice the wrong library name. 😆

erikras added a commit to sckoh/redux-form that referenced this pull request May 9, 2017
erikras pushed a commit that referenced this pull request May 9, 2017
…ializing the form (#2835)

* passing the last initialValues into initialize action if it is reinitializing the form

* Added test related to #2809

* Removed meta.touched from FieldArray props (#2836)

* Add props to the params documented in the field-level validations example (#2855)

* Fix typo in docs/api/Field.md (#2864)

* Improve SubmissionError docs (#2873)

* Add a selector to get touched and visited props from state (#2859)

* added a selector (getFormMeta) which retrieves the form.{$name}.fields slice from state

* added documentation for getFormMeta selector

* Fixed field-level validation reset bug (#2888)

* deleteInWithCleanUp needs to check if there is a value in the state before deleting. (#2876)

* deleteInWithCleanUp needs to check if there is a value in the state before deleting, to avoid Invalid KeyPath errors

* Got rid of string compare

* clean script - added es folder to be rimrafed before build (#2860)

* Add a Field's initial value to props.meta.initial (#2858)

* Add meta.initial to Field props for access to a field’s initial value.

* es6 typo

* add meta.initial to the docs for Field

* Demonstrated that calling onBlur() does not change value #2768

* Better cleanup on UNREGISTER (#2889)

* Checkbox default behavior proposal (#2863)

instead true/false to be true/'' as the current true/false breaks pristine prop

* Fancy new npm5 lock file!

* Allow direct imports to reduce bundle size. (#2893)

* Fix deep equal on maps with different structures (#2892)

* Remove references to react-router from doc page. (#2898)

* Add PropTypes for the Field components (#2862)

* Add PropTypes for the Field components

* Export fieldInputPropTypes, fieldMetaPropTypes and fieldPropTypes

* Merged with changes from #2893

* Exports defaultShouldValidate and defaultShouldAsyncValidate (#2891)

* Exports defaultShouldValidate and defaultShouldAsyncValidate

These default functions are exported for external use - particularly so
that a user can wrap the default functionality with specialised behaviour.

To resolve #2890

* Export default shouldValidates in immutable wrapper

* Allow direct imports to reduce bundle size. (#2893)

* Fix deep equal on maps with different structures (#2892)

* Remove references to react-router from doc page. (#2898)

* Add PropTypes for the Field components (#2862)

* Add PropTypes for the Field components

* Export fieldInputPropTypes, fieldMetaPropTypes and fieldPropTypes

* Merged with changes from #2893
@erikras
Copy link
Member

erikras commented May 9, 2017

Published as v6.7.0.

@piuccio
Copy link

piuccio commented May 11, 2017

screen shot 2017-05-11 at 16 23 56

Interesting PR, two minor things

  • You need a space in the markdown after ##, currently it looks like in the picture above
  • There's no mention on how to use this together with immutable.js

@ruiaraujo
Copy link
Contributor Author

@piuccio It is the same thing to use it with immutable, just add immutable.

@piuccio
Copy link

piuccio commented May 11, 2017

just add immutable

where?

redux-form/es/whatever/immutable
redux-form/es/immutable/whatever

I suppose it's the first one, but it would be nice to have it in the doc

@timhwang21
Copy link
Contributor

Hi, I'm noticing that the Babel plugin doesn't work well with actions:

WARNING in ./src/foo/bar.js
23:23-30 "export 'default' (imported as 'actions') was not found in 'redux-form/es/actions'

My import:

import { actions, Field, reduxForm } from 'redux-form';
const { reset } = actions;

This can be worked around from the user's end by changing the import statement, but I was wondering if it would be a good idea to add a default export to es/actions that contains all the actions. I think the import pattern I'm using is fairly standard, and the change would help users optimize their bundles with minimal refactoring.

Alternatively, if this is happening because I'm doing something wrong, please let me know. Thanks!

@brneto
Copy link
Contributor

brneto commented May 8, 2018

@erikras What are that createValues.js and values.js? Why it's not documented? Can I use it do access the form field values or you plan to remove that on the future?

@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants