-
Notifications
You must be signed in to change notification settings - Fork 272
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
chore: improve build responsiveness #2992
Conversation
} | ||
} | ||
|
||
const process = processes.find(p => p.cmd.includes("postcss") && p.cmd.endsWith(`-w --packageName=${packageName}`)); |
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.
better make this p.cmd.includes
which has the same effect but is less error prone in case someone adds more parameters to the postcss command
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 put endsWith
because @ui5/webcomponents
will also match @ui5/webcomponents-fiori
.
if (process) { | ||
await fkill(process.pid); | ||
|
||
exec(`npx nps build.styles.components`, (err, stdout, stderr) => { |
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.
isn't the watch below sufficient because a new start will see the newly added files?
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 turns out no, that's why probably we have first build.styles.components
and only then watch.styles.components
. The -w
flag does not execute the initial action.
console.log(stdout); | ||
}); | ||
|
||
exec(`npx nps watch.styles.components`, (err, stdout, stderr) => { |
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.
not sure if starting npx
from yarn
won't have additional problems. could we reuse the nps
api with require
?
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 didn't find any. That was my initial intention too. I could only find nps-utils
.
@@ -62,12 +64,14 @@ const getScripts = (options) => { | |||
es5: 'rollup --config config/rollup.config.js -w --environment ES5_BUILD,DEV,DEPLOY_PUBLIC_PATH:/resources/' | |||
}, | |||
styles: { | |||
default: 'concurrently "nps watch.styles.themes" "nps watch.styles.components"', | |||
default: 'concurrently "nps watch.styles.themes" "nps watch.styles.components" "nps watch.styles.monitor"', |
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.
watch and monitor are the same thing, something like restart-on-add
is more descriptive
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.
There are a couple of general problems:
- I still can't have it work on windows: the
ps-list
module ships a binary that simulates ps on windows, but it does not provide the full command, only the name of the executable "something.exe" without any parameters. Runningps
on the git bash is the same. - Sometimes the
ps-list
fails with a promise rejection on Mac, therefore the retry. The retry works, but once I had an error trying to kill the current postcss process.
This change fixes 2 bugs of the
yarn start
command:.css
files are not usable unlessyarn start
is killed and re-run. The reason is a knownpostcss
bug in--watch
mode: newly created files are not detected, only existing files are re-compiled (--watch mode doesn't pick up new files postcss/postcss-cli#161).messagebundle.properties
are not automatically available ini18n-defaults.js
and if you try to use them - the bundle breaks. This is due to the fact that there is no watch mode fori18n
.Changes:
chokidar
for watching thethemes/
directory and for each newly added file runpostcss
to build this file only, and then run againpostcss -w
to start watching it.watch
task fori18n
too: it only needs to watch themessagebundle.properties
file and regenerate the defaults file (not the.json
s).After the change it's possible to create a new component with all needed files and resources without restarting
yarn start
.