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

[feat] Adds support for large assets (> 100 MB) #3194

Merged
merged 6 commits into from Oct 27, 2019
Merged

[feat] Adds support for large assets (> 100 MB) #3194

merged 6 commits into from Oct 27, 2019

Conversation

sebiniemann
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

I didn't add any new tests, but expect the existing ones to be sufficient.

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Replaces hash.js with crypto (for NodeJS builds) and asmcrypto.js for browser builds to support assets with >100 MB.

After some benchmarks (see linked issue), crypto was chosen for NodeJS builds due to its (currently) superior performance. asmcrypto.js was then chosen instead of the Web Crypto API, as the digest function inside SubtleCrypto is async, while most of rollup is synchronous. Rewriting rollup for this seemed to be an overkill.

@sebiniemann
Copy link
Contributor Author

@lukastaegert I had to fix an additional problem(?) after running all tests.

In line

hash.update(options.format);

the format is directly added to the hash. Typescript however assumes that it can still be undefined at this point, which collides with update(data: string | Buffer).

Seeing many other lines which simply cast it to string, for example:

options.format as string,

I assume the the format being undefined is not actually possible at this point, given the current call path? Currently, I also added as string to this line, instead of surrounding it with an if clause.

@codecov
Copy link

codecov bot commented Oct 27, 2019

Codecov Report

Merging #3194 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3194      +/-   ##
==========================================
+ Coverage   90.19%   90.19%   +<.01%     
==========================================
  Files         165      167       +2     
  Lines        5905     5907       +2     
  Branches     1797     1797              
==========================================
+ Hits         5326     5328       +2     
  Misses        350      350              
  Partials      229      229
Impacted Files Coverage Δ
src/utils/crypto.ts 100% <100%> (ø)
src/utils/FileEmitter.ts 100% <100%> (ø) ⬆️
browser/crypto.ts 100% <100%> (ø)
src/Chunk.ts 92.89% <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 b123169...0ea074c. Read the comment docs.

@sebiniemann
Copy link
Contributor Author

sebiniemann commented Oct 27, 2019

There seems to be a problem with asmcrypto.js and modern V8 versions (see first circleci tests, NodeJS 12 builds).

After fixing the issue with Uncaught TypeError: Cannot read property '_stderr' of undefined to retrieve the actual error missing, it didn't really get much more useful:

(node:10968) V8: C:\Users\Sebi\Desktop\rollup\dist\shared\index.js:65 Invalid asm.js: Unexpected token

I am not sure what changed in V8 or asm.js that asmcrypto.js is now incompatible.

For now, NodeJS builds are stil capable to support large asset files by using the crypto library. For browser builds, I reverted it back to hash.js, cause it is synchronous in contrast to the Web Crypto API and also works under newer V8 versions, which may/are also used in browser.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Very nice, thanks a lot! Only one minor thing I would change.

browser/crypto.ts Outdated Show resolved Hide resolved
src/Chunk.ts Show resolved Hide resolved
@sebiniemann
Copy link
Contributor Author

Changed it to a paramter-less function :)

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Nice, that was quick! Thanks a lot!

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.

None yet

2 participants