-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Update light build targets to include Typescript extensions #2075
Update light build targets to include Typescript extensions #2075
Conversation
@tchakabam I think you've worked a lot on this, do you see any issues with this change? What prompted me looking was the build failures on |
Link #2070 |
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.
Looks good to me
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.
Should we just use webpack-merge? https://github.com/survivejs/webpack-merge
It's called out in the official webpack docs https://webpack.js.org/guides/production/
@johnBartos that package looks nice but brings in more dependencies, and what is here now is essentially the same thing ¯_(ツ)_/¯ |
True. We could also be more minimal and just add the extensions array to the returned value of |
I'm all for the smallest change possible that fixes the issue. I reverted the deep-merge in favour of simply defining the extensions array on the two My 2c would be if continues to be an issue going forwarded to maintain the simple approach, |
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 actually would prefer a more generic solution (previous commit or merge lib) over this one off fix, but this works
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.
👍
2643ebe
…mended merging library.
Squashed history down to just the generic webpack-merge implementation. It does do some good things where it will deep-merge loader arrays, and is the webpack approved solution for this problem, so I went with that. Thanks everyone. |
This fixes an issue where light-dist / light defined their own resolve property causing them to not get the base config resolve for extensions, etc.
Includes a few minor lint fixes that popped in my editor as well.
Before:
After: