-
Notifications
You must be signed in to change notification settings - Fork 331
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
Ensure "clean" target matches --destination
flag
#2877
Conversation
ba3dc1a
to
50518a8
Compare
50518a8
to
3ce4d6d
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.
One tiny question. This looks great to me!
It's probs worth getting a js squad dev to look over this still as I'm rusty on these build scripts and conventions that we're trying to follow with these tooling updates.
3ce4d6d
to
84af53d
Compare
84af53d
to
1632ce5
Compare
1632ce5
to
7f04385
Compare
7f04385
to
50dcb2e
Compare
// Defaults for known tasks | ||
const destinations = [ | ||
{ | ||
task: 'build:package', |
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.
Part of me wants to do this by parsing the task name, but this is way less fussy and prone to errors!
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 think this looks grand!
Maybe at this stage it makes sense to keep the --destination
flag until we've completed work to scope our JavaScript/Browser support changes, in case we make some big changes that require liberal use of it.
I think that's unlikely to happen, but it probably makes sense to keep our options open, and we can always come back and get rid of it down the line.
Thanks @domoscargin Did you see the "remove" commit was 1 of 3? I'm more than happy to drop that one, keeping the automatic |
Yup, sorry, should've been clearer - I think we should do exactly that. Was just holding off approving to see if you were on the same page. I'll approve now and we can drop that commit. |
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.
🏆
--destination
flag override--destination
flag
50dcb2e
to
6e87d80
Compare
I noticed when we build to a custom directory via
--destination
it's not "cleaned"☝️ As those two commands only clean ./package via
cleanPackage()
and ./dist viacleanDist()
This PR includes:
build:package
orbuild:dist
--destination
if we want to keep itRemoval of--destination
if we want to get rid of itThis comment was really useful: #2243 (comment)