Skip to content
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(bin): extension detection (#724) #728

Merged
merged 5 commits into from
Jan 7, 2019

Conversation

lakatostamas
Copy link
Contributor

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
No, but I created test repositories:
https://github.com/lakatostamas/webpack-babel
https://github.com/lakatostamas/webpack-ts

Summary
Issue: #724
This commit introduced a bug in the default config file discovery: b61220e#diff-daabbe74e88640829a3ad9e9899efd8cR79
Before this commit, the extension of webpack.config.ts was .ts in the default configFile discovery, but after this commit, it became just ts (without .). webpack.config.babel.js extension is js now, but it should be .babel.js. With this pull request, I introduce a fix, which gets the name of the default config file and removes the webpack.config or webpackfile from it.
I don't know how to add tests when the extension of webpack config is not js. I haven't found any example, but if you tell me how to do it, I would happily add some tests to cover these cases.

Does this PR introduce a breaking change?
No

@zzh8829
Copy link

zzh8829 commented Jan 5, 2019

This should fix the problem of .babel.js discussed in webpack/webpack#5960
Looks like the error was actually introduced in 14ec0a5#diff-daabbe74e88640829a3ad9e9899efd8cR90

@ematipico
Copy link
Contributor

We should add a bin test for that, if possible

@webpack-bot
Copy link

@lakatostamas Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@lakatostamas
Copy link
Contributor Author

@ematipico Thanks for the review! The webpack-babel-config test works because in the project root we have a .babelrc and @babel/preset-env is installed. I added a .babelrc to webpack-babel-config root to be more explicit, but this test will fail if the preset-env dependency won't be installed.
If we want to add webpack.config.ts related tests, then we have to add ts-node as a devDependency, but I think it's not worth for one test.

@ematipico
Copy link
Contributor

I personally prefer install an additional devDependency and make sure that we don't break those kind of scenarios. After all this is a package used by lot of people :) what do you think?

@lakatostamas
Copy link
Contributor Author

Okay, that makes sense. I added webpack.config.ts test

@webpack-bot
Copy link

@lakatostamas The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from travis (failure) and fix these issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants