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

Fixed bundling ReactDOM #2

Merged
merged 3 commits into from
Jul 17, 2018
Merged

Conversation

Andarist
Copy link
Contributor

No description provided.

@@ -1,31 +1,29 @@
{
"name": "react-final-form-html5-validation",
"version": "1.0.0",
"description":
"A swap-in replacement for 🏁 React Final Form's <Field> component to provide HTML5 Validation",
"description": "A swap-in replacement for 🏁 React Final Form's <Field> component to provide HTML5 Validation",
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 think my npm formats package.json when doing a fresh install for some reason, no idea how to stop it (havent searched for it though 😉 ). Hope it's ok

@@ -71,13 +69,19 @@
"final-form": ">=4.0.0",
"prop-types": "^15.6.0",
"react": "^15.3.0 || ^16.0.0-0",
"react-dom": "^15.3.0 || ^16.0.0-0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was added! this peer dep was already required, only not in package.json

plugins: [
resolve({ jsnext: true, main: true }),
flow(),
commonjs({ include: 'node_modules/**' }),
umd && commonjs({ include: 'node_modules/**' }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just extra guard, for non-umd build u dont want to use this plugin at all

package.json Outdated
@@ -69,7 +69,6 @@
"final-form": ">=4.0.0",
"prop-types": "^15.6.0",
"react": "^15.3.0 || ^16.0.0-0",
"react-dom": "^15.3.0 || ^16.0.0-0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be kept

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI is failing now because this got removed

@codecov
Copy link

codecov bot commented Jul 17, 2018

Codecov Report

Merging #2 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #2   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          37     37           
  Branches       16     16           
=====================================
  Hits           37     37

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 3f21321...953a2e3. Read the comment docs.

@erikras
Copy link
Member

erikras commented Jul 17, 2018

Any proof that this reduces bundle size? My initial test was pretty equivalent to master.

@Andarist
Copy link
Contributor Author

I've made tests after running ./node_modules/.bin/nps build.es and then inspecting the size with wc -c dist/react-final-form-html5-validation.es.js

master 670401
my PR 6402

Rollup build times also went down quite a bit (1.8s vs 422ms)

@Andarist
Copy link
Contributor Author

The problem was using findDOMNode but not specifying react-dom as external

@Andarist Andarist mentioned this pull request Jul 17, 2018
@erikras
Copy link
Member

erikras commented Jul 17, 2018

Um, confirmed. From 30 kb down to 1.45kb. 🤦‍♂️💥

@erikras erikras merged commit 841dce0 into final-form:master Jul 17, 2018
@Andarist Andarist deleted the fix/bundle-bloat branch July 17, 2018 21:16
@erikras
Copy link
Member

erikras commented Jul 18, 2018

Published in v1.0.1.

dist/react-final-form-html5-validation.es.js went from 730K to 5.4K. 😱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants