-
-
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
fix(custom-webpack): support exporting function in config written in TS #627
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, that was indeed missing. @arturovt can you point out to the tests please?
@all-contributors please add @a1russell for bug and code |
I've put up a pull request to add @a1russell! 🎉 |
@a1russell hey, that would be great if you'd used the original PR template, thanks!
Go to
Could you provide a working example? |
@a1russell change the commit name to
to follow the conventional commits notation. |
Edited original description to add back template.
Thanks. I'll give this a go.
I was referring to the syntax mentioned in the comment above the code I edited. I honestly am not sure what a working example would look like for that. |
1e3cc34
to
779334f
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.
Thank you for changing PR description. LGTM. Just awaiting CI.
779334f
to
bdb2b5f
Compare
Need to update the documentation in order to reflect the changes. |
What documentation would need to be updated, please? |
I'd say this is the most appropriate place: https://github.com/just-jeb/angular-builders/tree/master/packages/custom-webpack#custom-webpack-config-function. |
13f9741
to
eba173c
Compare
packages/custom-webpack/README.md
Outdated
|
||
|
||
```typescript | ||
import { CustomWebpackBrowserSchema } from '@angular-builders/custom-webpack/dist'; |
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.
@just-jeb should this be used publicly?
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.
@arturovt I don't see any problem with that.
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.
Maybe we could expose it publicly, thus it won’t be imported from the dist folder?
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.
Sure, up until TS support there was no need in exporting anything but now it totally makes sense.
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.
Actually it is already exported, so we just need to set "types": "dist/index.d.ts"
in package.json
(I believe it should work event without it, it should be the default entry)
eba173c
to
ac37ee2
Compare
ac37ee2
to
7f67c22
Compare
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?
I noticed that TypeScript support was recently added, so I tried it out. It didn't seem to work with my config, and I am pretty sure it's because I am exporting a function rather than an object. The code that was added to support TypeScript only seems to test for objects before my PR.
Also, apologies, I couldn't find any tests to modify to account for my fix. You'll have to point them out to me if they exist.
What is the new behavior?
Exported function works with TypeScript config files.
Does this PR introduce a breaking change?
Other information
I should also point out that I couldn't get
module.exports
syntax to work at all. I think that would require change to the tsconfig, e.g.esModuleInterop
or something as described on the Webpack documentation. I didn't try to fix that, though.