-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Split InternalAsset into UncommittedAsset and CommittedAsset (Cache ASTs followup) #4378
Conversation
|
Benchmark Resultspackages/benchmarks/three 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... packages/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 BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... packages/benchmarks/ak-editor 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... |
@@ -24,6 +24,7 @@ | |||
"@babel/plugin-transform-typescript": "^7.4.5", | |||
"@babel/preset-env": "^7.0.0", | |||
"@babel/traverse": "^7.0.0", | |||
"@parcel/babel-ast-utils": "^2.0.0-alpha.3.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the import/no-extraneous-dependencies
lint rule didn't catch this. It seems to only be missed with packages in the monorepo?
}; | ||
} | ||
|
||
export async function getConfig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only expose this in transformers IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getPackage
is probably useful everywhere though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validators receive an Asset
(not a MutableAsset
unlike transformers), and use .getConfig()
😕
|
||
const generateResults: WeakMap<Asset, Promise<GenerateOutput>> = new WeakMap(); | ||
|
||
export function generateFromAST( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only needed in CommittedAsset? Should we make this and the below function methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we generate early in Transformation.js
if we're not scope hoisting, which has an UncommittedAsset
.
fcc3d52
to
53b94d2
Compare
53b94d2
to
9d4cf94
Compare
This is a followup based on my experience implementing parts of #3695, and also integrates some feedback from that PR.
This splits
InternalAsset
intoUncommittedAsset
andCommittedAsset
. The former implements a mutable interface for transformers, only holding into values in memory (with the exception of generation), and the latter implements an immutable interface that loads content from disk and caches it in memory.Previously,
InternalAsset
implemented both, in some situations branching on the state. This also allows for simpler implementations in each case. Common code was extracted intoassetUtils.js
in the same directory.This also cleans up
babel7.js
to safely usegetAST()
and moves it to use theAsync
methods as we don't require synchronous behavior.Test Plan:
yarn test