Encoded inline bundles #3726
Merged
Encoded inline bundles #3726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wbinnssmith
requested review from
DeMoorJasper,
devongovett,
gorakong and
padmaia
|
Re: detecting binary files, https://www.npmjs.com/package/isbinaryfile has an extra step that counts characters it considers "weird". We could use that module here instead. |
Benchmark Resultspackages/benchmarks/kitchen-sink 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... packages/benchmarks/react-hn ✅
Timings
Cold Bundles
Cached BundlesNo bundle changes detected. packages/benchmarks/ak-editor ✅
Timings
Cold Bundles
Cached Bundles
|
wbinnssmith
force-pushed
the
wbinnssmith/inline-bundle-urls
branch
8 times, most recently
from
2a8a310
to
4e7fa48
Compare
wbinnssmith
force-pushed
the
wbinnssmith/inline-bundle-urls
branch
9 times, most recently
from
2f146d0
to
456afb9
Compare
devongovett
reviewed
Nov 6, 2019
| if (dependency.moduleSpecifier.includes(':')) { | ||
| [pipeline, filePath] = dependency.moduleSpecifier.split(':'); | ||
| let transformsWithPipelines = {}; | ||
| for (let key of Object.keys(this.config.transforms)) { |
There was a problem hiding this comment.
Should we also include optimizers here? What if you had a named pipeline that only existed in the optimizer config?
There was a problem hiding this comment.
Good point. Think we should be using the config matching method or just extract the pipelines ourselves here and match against them?
There was a problem hiding this comment.
Perhaps we could extract the named pipelines once and cache them? Maybe in the ParcelConfig class?
packages/core/utils/src/string.js
Outdated
| @@ -0,0 +1,9 @@ | |||
| // @flow strict-local | |||
|
|
|||
| export function escapeJSStringLiteral(str: string): string { | |||
There was a problem hiding this comment.
Maybe just use JSON.stringify?
| : packagedBundle.contents | ||
| ).toString(); | ||
|
|
||
| let inlineType = nullthrows(entryBundle.getMainEntry()).meta.inlineType; |
There was a problem hiding this comment.
Where do we define what inlineType values are allowed? I'm not sure it should be enforced in this util in case new ones need to be defined by plugins.
There was a problem hiding this comment.
It's not really enforced. inlineType isn't a formal concept in core and is just something that transformers and packagers agree on.
There was a problem hiding this comment.
it's enforced by the line below asserting the values...
There was a problem hiding this comment.
This function isn't run in core though, it's run in packagers. Perhaps this util shouldn't live in core/utils?
There was a problem hiding this comment.
Yeah true, but the util is still kinda “shared” code that shouldn’t make assumptions if it’s meant to be informal.
wbinnssmith
force-pushed
the
wbinnssmith/inline-bundle-urls
branch
from
456afb9
to
e1f2a1a
Compare
added 9 commits
November 6, 2019 13:41
wbinnssmith
force-pushed
the
wbinnssmith/inline-bundle-urls
branch
from
e1f2a1a
to
b189929
Compare
added 7 commits
November 13, 2019 14:12
wbinnssmith
force-pushed
the
wbinnssmith/inline-bundle-urls
branch
from
c3ee1c0
to
449490b
Compare
devongovett
approved these changes
Nov 14, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #3718.
This implements the ability to inline the content of one bundle into another by using protocol-style imports in the form of
data-url:./img.pngto receive a data url orbundle-text:./worker.jsto receive the literal text of the compiled bundle content.This is accomplished by adding
transformer-inline, which setsisInlineto true on assets, attaching the requested pipeline name to assets and bundles, and adding an optimizer to transform bundle contents into data urls. References to the inline bundle are left behind in the form of bundle ids (this may change with css), and the packaged inline bundles are then replaced during packaging by invoking the inline packaging api.For data urls, the mimetype is automatically determined based on the bundle filepath, and the content is encoded either as text for non-binary content, or as base64 for binary content (content with a 0x00 byte present).
Question: Is there a more robust heuristic for determining binary content than checking for the presence of a 0x00 byte?
Remaining tasks:
Test Plan: Added integration tests.