-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃彈 Lazy building 3p integrations #32772
Conversation
Hey @rsimha! These files were changed:
|
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.
One preliminary comment below to make this code more efficient. Happy to take another look.
88f5008
to
a80a584
Compare
08acda6
to
7337a2a
Compare
@rsimha updated to fix conflicts. mind take another look? |
7337a2a
to
328a7ed
Compare
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.
Almost there. A couple comments below.
build-system/server/lazy-build.js
Outdated
@@ -15,18 +15,24 @@ | |||
*/ | |||
'use strict'; | |||
|
|||
const {generateBundles} = require('../tasks/3p-vendor-helpers'); |
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.
Consolidate this with the other require
statement from the same file.
build-system/server/lazy-build.js
Outdated
const argv = require('minimist')(process.argv.slice(2)); | ||
const { | ||
doBuildExtension, | ||
maybeInitializeExtensions, | ||
getExtensionsToBuild, | ||
} = require('../tasks/extension-helpers'); | ||
const {buildVendorLazily} = require('../tasks/3p-vendor-helpers'); |
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.
Consistency nit: The "lazy" part comes from how the function is called and not what it does. Can we name this doBuild3pVendor
to be consistent with doBuildJs
?
? new RegExp(`\\/dist\\.3p\\/${VERSION}\\/vendor\\/([^\/]*)\\.js`) // '/dist.3p/21900000/vendor/*.js' | ||
: /\/dist\.3p\/current\/vendor\/([^\/]*)\.max\.js/; // '/dist.3p/current/vendor/*.max.js' |
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.
Nice!
} | ||
|
||
/** | ||
* Generate bundles for all JS to be built. |
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.
nit: Should this be Generate bundles for all 3p vendors to be built
?
328a7ed
to
36f8850
Compare
@rsimha thanks! updated. |
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.
LGTM!
Currently, 3p integrations cannot be lazily built. This PR fixes the lazy building so that
gulp
can defer building 3p integration JS files till needed.