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

fix: make a single webpack bundle #203

Merged
merged 1 commit into from
Mar 6, 2018
Merged

fix: make a single webpack bundle #203

merged 1 commit into from
Mar 6, 2018

Conversation

vmx
Copy link
Member

@vmx vmx commented Mar 4, 2018

Prior to this commit there was a webpack bundle created for every test
file. That lead to very high memory usage when running the Browser tests.
Now there's a single bundle.

@codecov
Copy link

codecov bot commented Mar 4, 2018

Codecov Report

Merging #203 into master will increase coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #203     +/-   ##
=========================================
+ Coverage   78.16%   78.47%   +0.3%     
=========================================
  Files           6        6             
  Lines         142      144      +2     
=========================================
+ Hits          111      113      +2     
  Misses         31       31
Impacted Files Coverage Δ
src/config/webpack/index.js 100% <100%> (ø) ⬆️

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 57c9554...f06086c. Read the comment docs.

@daviddias
Copy link
Member

@vmx will .spec continue to work?

@vmx
Copy link
Member Author

vmx commented Mar 5, 2018

@diasdavid Yes. This change should be backwards compatible. It will run all *.spec.js and browser.js (in case it exists). You can also supply a specific file on the command line.

@vmx
Copy link
Member Author

vmx commented Mar 5, 2018

I've tested it locally with js-ipfs-api, js-ipfs and js-ipld-zcash.

@dryajov
Copy link
Member

dryajov commented Mar 5, 2018

@vmx I have a suggestion - we can test this on CI's as well by using this branch as a git url in the package.json of a project. I think it would be a good way of making sure it works on CI without an actual release. js-ipfs-api should be a good candidate as it's the one that fails the most often.

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

one note, other than that I think this looks pretty good

// that's a single entry point, but it could also be a single test that
// is given as command line parameter
const preprocessors = getPatterns(ctx).reduce((acc, pattern) => {
acc[pattern] = ['webpack']
Copy link
Member

Choose a reason for hiding this comment

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

missing 'sourcemap' preprocessor

Prior to this commit there was a webpack bundle created for every test
file. That lead to very high memory usage when running the Browser tests.
Now there's a single bundle.
@vmx vmx force-pushed the karma-webpack-bundle-alt2 branch from 9a10fb6 to f06086c Compare March 5, 2018 22:16
@vmx
Copy link
Member Author

vmx commented Mar 5, 2018

Thanks @dignifiedquire, I (force) pushed a new version.

@dryajov Good suggestion, I create PR on js_ipfs-api using this PR: ipfs-inactive/js-ipfs-http-client#705.

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Ship it! ❤️

@vmx
Copy link
Member Author

vmx commented Mar 6, 2018

As it got approved by @diasdavid and I fixed what @dignifiedquire suggested I dare to merge it. If it really turns out breaking things, we can always revert it.

@vmx vmx merged commit f1e2850 into master Mar 6, 2018
@daviddias daviddias deleted the karma-webpack-bundle-alt2 branch March 6, 2018 18:22
@daviddias
Copy link
Member

@vmx you have publish permissions. Go for it :)

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.

4 participants