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
dev: Improve prod build and monorepo scripts #165
Conversation
Feel free to clone and run |
b683cbe
to
df80a74
Compare
Pull Request Test Coverage Report for Build 378
💛 - Coveralls |
df80a74
to
7465bed
Compare
@@ -12,7 +12,7 @@ | |||
"presets": [ | |||
["env", { | |||
"targets": { | |||
"browsers": ["last 2 versions", "ie 11"] | |||
"browsers": ["> 5%"] |
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.
This should slim down the build. 👍
package.json
Outdated
"watch:buildpack": "cd packages/pwa-buildpack && npm run -s watch", | ||
"watch:peregrine": "cd packages/peregrine && npm run -s watch", | ||
"watch:dev-server": "nodemon --exec npm --cwd packages/venia-concept start", | ||
"watch:venia": "concurrently -c yellow.dim,green.dim,cyan.dim -n ␢,℗,☄︎ 'npm run watch:buildpack' 'npm run watch:peregrine' 'npm run watch:dev-server'", |
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 really the plan? Watch all of these at the same time? I'd rather just watch venia
, personally. Can we make this one watch:all
or something?
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.
This is definitely what you want. This has the exact same functionality that npm start
in venia does today, with three key improvements:
- If you change a Peregrine source file (e.g. moving a concept from Venia to Peregrine) it will hot reload and stay up to date, instead of requiring a manual Peregrine rebuild
- If you change a Buildpack source file (e.g. changing the behavior of a plugin or DevServer), it will shut down the dev server, rebuild Buildpack, and restart the dev server to reflect the new changes.
- You can run it from project root.
If you don't touch Peregrine or Buildpack, it's functionally the same.
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.
If you don't touch peregrine
or buildpack
, is it more expensive to watch them? Does it bog your system down more? That's my only concern.
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.
@jimbo Peregrine and Buildpack have many fewer files than Venia, so I don't think it's a major concern. However: I could just change this so that "watch:venia" just runs "cd packages/venia-concept && npm start; cd -", and the command currently named "watch:venia" could be renamed "watch:all". Would that help?
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.
Yes, I think that would help. 👍
// In production mode, it may be a pathname, which makes it unsafe | ||
// to use as an API base. Normalize it as a full path using a DOM node | ||
// as a native URL parser. | ||
const parser = document.createElement('a'); |
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.
Can you elaborate? What does this workaround do that URL doesn't?
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's guaranteed to produce an absolute URL from a possibly-relative URL. The webpackPublicPath
argument is relative in production and absolute in dev, so we need something that can handle both. URL objects cannot:
// URL constructors deal in absolute URLs.
try {
new URL('/hermano')
} catch(e) {
console.warn(e.message, '/hermano'); // Failed to construct 'URL': Invalid URL
}
// They can use relative URLs if you supply a "base URL" as a second argument.
const absoluteBase = 'https://github.com/magento-research/pwa-studio/';
const url = new URL('pulls/165', absoluteBase);
console.log(url)
// Url { protocol: "https:" hostname: "github.com", pathname: "/magento-research/pwa-studio/pulls/165" }
// If the base URL is relative, they throw.
try {
new URL('pulls/165', '/magento-research/pwa-studio/');
} catch(e) {
console.warn(e.message, '/magento-research/pwa-studio/');
}
// Anchor tags will accept absolute or relative 'href' attributes:
const parserTag = document.createElement('a');
parserTag.setAttribute('href','/magento-research/pwa-studio');
// But their properties are always absolute:
console.log('parserTag.getAttribute("href")', parserTag.getAttribute("href"));
console.log('parserTag.href', parserTag.href); // "https://github.com/magento-research/pwa-studio/";
// So you can guarantee an absolute URL built with URL semantics
// if you can guarantee an absolute base, which we do with our parser.
// Which is a lot safer than string concatenation.
function appendPathToUrl(path, base) {
const parser = document.createElement('a');
parser.setAttribute('href', base);
return new URL(path, parser.href).toString()
}
console.log(
'true or false: using an anchor tag as a parser normalizes absolute URLs:'
appendPathToUrl('pulls/165', '/magento-research/pwa-studio/') ===
appendPathToUrl('pulls/165', 'https://github.com/magento-research/pwa-studio/')
);
// true
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.
I see, makes perfect sense to me now. Thanks for the explanation. 👍
} | ||
browsers: ['> 5%'] | ||
}, | ||
modules: false |
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 a bug fix?
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.
Yep! For Webpack tree shaking.
7465bed
to
26c5c0b
Compare
@jimbo Made changes based on your feedback. |
26c5c0b
to
c20fc36
Compare
@jimbo Build is fixed. We had a misconfiguration in CI where the test suite wouldn't run anymore, which is how the Greenkeeper update got merged without noticing test failure.
|
- Add more root-directory usability to monorepo scripts - Add `npm run watch:all` mode that: - Simultaneously builds buildpack, peregrine, and venia - Hot reloads venia if either peregrine or venia change on disk - Fully restarts dev server if buildpack changes on disk - Interleaves and prefixes output - Doesn't use lerna to do this, because lerna's parallelizing console mode is not customizable enough - Fix production builds (by pegging them to very new browsers)
c20fc36
to
e26a97c
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.
@zetlen Thanks for making changes. Looks good to me now.
For what it's worth, watch:all
is very slow compared to watch:venia
. It makes my machine's fans spin up to high, and it takes a long time to hot reload. That can be optimized, so I don't think it blocks this, but it's something we'll need to improve or remove.
- Add more root-directory usability to monorepo scripts - Add `npm run watch:all` mode that: - Simultaneously builds buildpack, peregrine, and venia - Hot reloads venia if either peregrine or venia change on disk - Fully restarts dev server if buildpack changes on disk - Interleaves and prefixes output - Doesn't use lerna to do this, because lerna's parallelizing console mode is not customizable enough - Fix production builds (by pegging them to very new browsers)
- Add more root-directory usability to monorepo scripts - Add `npm run watch:all` mode that: - Simultaneously builds buildpack, peregrine, and venia - Hot reloads venia if either peregrine or venia change on disk - Fully restarts dev server if buildpack changes on disk - Interleaves and prefixes output - Doesn't use lerna to do this, because lerna's parallelizing console mode is not customizable enough - Fix production builds (by pegging them to very new browsers) fix: improve dev mode watch and log
- Add more root-directory usability to monorepo scripts - Add `npm run watch:all` mode that: - Simultaneously builds buildpack, peregrine, and venia - Hot reloads venia if either peregrine or venia change on disk - Fully restarts dev server if buildpack changes on disk - Interleaves and prefixes output - Doesn't use lerna to do this, because lerna's parallelizing console mode is not customizable enough - Fix production builds (by pegging them to very new browsers) fix: improve dev mode watch and log chore: remove greenkeeper for now chore: flush CircleCI cache by revving cache key chore: reorg dependencies for lerna fix: lerna dedupe and hoist fix: use npm ci command to ensure deps fix: populate .env file in ci fix: clean after build for CI fix: only clean dist, leave node_modules fix: help eslint detect root modules
- Add more root-directory usability to monorepo scripts - Add `npm run watch:all` mode that: - Simultaneously builds buildpack, peregrine, and venia - Hot reloads venia if either peregrine or venia change on disk - Fully restarts dev server if buildpack changes on disk - Interleaves and prefixes output - Doesn't use lerna to do this, because lerna's parallelizing console mode is not customizable enough - Fix production builds (by pegging them to very new browsers) fix: improve dev mode watch and log chore: remove greenkeeper for now chore: flush CircleCI cache by revving cache key chore: reorg dependencies for lerna fix: lerna dedupe and hoist fix: use npm ci command to ensure deps fix: populate .env file in ci fix: clean after build for CI fix: only clean dist, leave node_modules fix: help eslint detect root modules
This PR is a:
[ ] New feature
[x] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation
Summary
npm run watch:venia
mode that:mode is not customizable enough
Additional information
Production builds will need further optimization in the future.
TODO: Adjust production build for middle tier concept
TODO: Use performance analysis to optimize chunk layout