Skip to content

Asset emission reworking#30

Merged
guybedford merged 20 commits intomasterfrom
refactoring
May 15, 2019
Merged

Asset emission reworking#30
guybedford merged 20 commits intomasterfrom
refactoring

Conversation

@guybedford
Copy link
Copy Markdown
Contributor

@guybedford guybedford commented May 9, 2019

This PR represents a reworking of the asset emission algorithm including:

  • Removing the "not found" require handling and instead relying on not found errors being runtime errors in ncc itself via New analysis updates ncc#378.
  • Refactoring of asset emission handling to avoid a number of special cases including the need to special case ffmpeg (which has tests here)
  • escapeNonAnalyzableRequires is no longer an option, and this is done much more consistently for both require and require.resolve. As a result many previously non-working dynamic require cases should now work out correctly.
  • Implementing wildcard asset handling, and process.cwd() analysis - Asset trigger extension and wildcards #10

The exact emission logic is described below:

process.cwd

The path.resolve and process.cwd() analysis will use the process.cwd of the build process itself. So if the build is run from the wrong directory these can not work out and cause assets not to be emitted.

To support a custom cwd, this can be passed as an option as well.

require.resolve

  • ALWAYS escape it as __non_webpack_require__.resolve.
  • Inline its expression wherever possible further.

require

  • If we're unable to analyze the input, escape it as __non_webpack_require__.
  • If the input is known but not webpack analyzable (utilized getKnownBinding), then inline the analyzed value (including in branched cases).
  • On the Webpack side, ensure that all require calls fallback to outer (like we did previously).
  • In addition ensure that any not found resolutions do not fail.

variable declarations and assignments

  • All variable declarations and assignments are analyzed for pure static values
  • If the value is an absolute path string, then it is allowed to become an asset emission
  • Otherwise, the walking routine will continue to check for internal asset triggers
  • The logic here has been significantly improved, with a lot of special casing removed. Even 'require()' is handled as a standard call expression analysis now in a variable declaration.

call expressions

  • All call expression callee values are statically analyzed to determine if they are known (typically from the known staticModules that might be required such as fs functions, path functions).
  • If the callee is a known pure function it is executed in the analysis
  • If the callee has a [TRIGGER] property, then it can trigger an asset emission (eg in path.resolve('./a')), the path.resolve function has a [TRIGGER] for emission like this, so we don't analyze every "./a" string).
  • If the callee is a well-known symbol, then we have various special cases applied (eg EXPRESS gets its own treatment this way etc).
  • Well known symbols are also applied for all fs functions as asset emission points. In this way fs.readFile(...) will emit the argument as an asset if it can be computed.

This PR should now resolve the following issues on ncc core:

Just a few ncc bugs left before this should be ready for further review and merging.

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 10, 2019

Codecov Report

Merging #30 into master will increase coverage by 2.04%.
The diff coverage is 75.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage    70.3%   72.34%   +2.04%     
==========================================
  Files          10       10              
  Lines         899     1034     +135     
==========================================
+ Hits          632      748     +116     
- Misses        267      286      +19
Impacted Files Coverage Δ
src/utils/wrappers.js 97.95% <100%> (+0.08%) ⬆️
src/utils/special-cases.js 77.77% <28.57%> (-17.47%) ⬇️
src/utils/binary-locators.js 18.42% <33.33%> (-0.5%) ⬇️
src/utils/static-eval.js 68.62% <69.32%> (+8.72%) ⬆️
src/asset-relocator.js 80.69% <83.66%> (+0.82%) ⬆️

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 b0520b5...89ecdc5. Read the comment docs.

@guybedford guybedford changed the title WIP: Asset emission reworking [do not merge] Asset emission reworking [do not merge] May 11, 2019
@guybedford guybedford changed the title Asset emission reworking [do not merge] Asset emission reworking May 14, 2019
@guybedford
Copy link
Copy Markdown
Contributor Author

ncc tests are passing against this PR. Merging for a beta release to gain further feedback.

@guybedford guybedford merged commit 0ae9ba7 into master May 15, 2019
@guybedford guybedford deleted the refactoring branch May 15, 2019 02:58
@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants