-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
[Enhancement] Support TS for indexTransform #689
[Enhancement] Support TS for indexTransform #689
Conversation
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.
Thanks for contribution! Overall looks good, left a few comments please address them.
Also please add and e2e test. You should test the specific use case that was failing (no webpack.config
defined in angular.json
but indexTransform
is defined and it's a TS file).
The best way to do that is:
- Add another configuration in this that defines
indexTransform
with TS file but not custom webpack config. - Add a test case similar to this but with corresponding configuration name.
- Add a protractor config similar to this but with the appropriate spec name. Also, instead of
specs.push
use `specs = ['your-regex']. - Add a run for this e2e test in the ci script while using
--configuration=your-config
option instead of--prod
.
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.
Thanks for the great job! Just two more comments to address and I'm merging.
@@ -0,0 +1,7 @@ | |||
export default (targetOptions, indexHtml) => { |
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.
Since it's a TS example we should showcase the types usage here.
Please import the relevant types from @angular-builders/custom-webpack
(if needed then reexport from @angular-devkit
).
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.
In my opinion, use static type for indexHtml
is good, but the other one is unnecessary, try to import a type and use for readonly obj just make our code more complicated. So, in that case, I think write document(readme) for it is enough.
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 disagree, having a type for these options is very helpful and gives you the exact set of options you can use. Otherwise you should go back and forth to the documentation (given it's up to date).
If it's a problem I can add the type myself.
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.
Haha. It's no problem to me 😄 , just discuss and raise my concerns. Moreover, you are the owner, you can ask me to do everything because I happy to contribute to your package and willing to follow your rules. So don't talk to me that way If it's a problem I can add the type myself.
.
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.
And one more concern, I think indexHtml
is more important than targetOptions
, so it should be the first param, some cases we no need to use targetOptions
. Is it possible to have a breaking change in the next release?
reimport dependencies
@just-jeb Please help me to check again. Thanks! |
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.
All good, merging!
Btw, what is itwcw
exactly?
@all-contributors please add @vixnguyen for code. |
I've put up a pull request to add @vixnguyen! 🎉 |
@just-jeb |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #687
Does this PR introduce a breaking change?