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

Update build tools #227

Merged
merged 4 commits into from
May 17, 2017
Merged

Update build tools #227

merged 4 commits into from
May 17, 2017

Conversation

thiamsantos
Copy link
Contributor

Sorry if any of the changes are out of scope.

CHANGELOG

@codecov-io
Copy link

codecov-io commented Apr 24, 2017

Codecov Report

Merging #227 into master will increase coverage by 16.91%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #227       +/-   ##
===========================================
+ Coverage   68.38%   85.29%   +16.91%     
===========================================
  Files           5        5               
  Lines         136      136               
===========================================
+ Hits           93      116       +23     
+ Misses         43       20       -23
Impacted Files Coverage Δ
src/diff.js 100% <ø> (+92%) ⬆️

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 385f41c...735499e. Read the comment docs.

Copy link
Collaborator

@imevro imevro left a comment

Choose a reason for hiding this comment

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

Awesome, will merge after fixes

package.json Outdated
"prepublish": "npm run clean && npm test && npm run build",
"uglify": "uglifyjs dist/index.umd.js -cm -o dist/index.umd.min.js",
"rollup": "node rollup.config.js"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can move scripts as they appears? For example, build run rollup, so rollup must be higher than build

rollup.config.js Outdated
@@ -0,0 +1,55 @@
const rollup = require('rollup').rollup;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need non-minified versions, so you can produce minified versions right here

Copy link
Contributor Author

@thiamsantos thiamsantos Apr 26, 2017

Choose a reason for hiding this comment

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

Sure. But I'm still in doubt if I should bundle two different versions, one using UMD and another CommonJS modules. Maybe just one is enough. What do you think @evgenyrodionov ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think UMD is enough

@thiamsantos
Copy link
Contributor Author

thiamsantos commented May 11, 2017

Any thoughts @evgenyrodionov?

@imevro
Copy link
Collaborator

imevro commented May 12, 2017

Looks good, but I need test it. On Monday, I guess.

@thiamsantos
Copy link
Contributor Author

I hope you don't mind @evgenyrodionov. I am taking advantage of this pull request to add some tests.

@imevro imevro merged commit f3e673c into LogRocket:master May 17, 2017
@imevro
Copy link
Collaborator

imevro commented May 17, 2017

Thank you! It looks pretty awesome.

@thiamsantos thiamsantos deleted the build-tools branch May 17, 2017 11:27
@thiamsantos
Copy link
Contributor Author

Thanks @evgenyrodionov!!!

@thiamsantos
Copy link
Contributor Author

Expose files using ES2015 modules through the properties pkg.module and pkg.jsnext:main.

As I mentioned above if you use a module bundler like webpack or rollup, it will get the code from the source files. That can cause problems like #229 when you don't use the babel plugin to handle the object spread operator. So I think that we can either explain that on readme and ask for people use the plugin or just expose the bundled file as was made before and prevent these problems. What do you think @evgenyrodionov?

@imevro
Copy link
Collaborator

imevro commented May 17, 2017

Object spread operator already fixed in 3.0.3, it's stage 3 proposal (candidate), so it was my mistake.

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.

3 participants