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: allow unknown files to use default require as fallback #1747

Merged
merged 2 commits into from Aug 19, 2020

Conversation

anshumanv
Copy link
Member

What kind of change does this PR introduce?
fix

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
No need

Summary

  • Remove unnecessary filtering
  • We filter all the config based on the available extensions in interpret which is not right and files should fallback to the default node require when they don't have a module needed.

Does this PR introduce a breaking change?
No

Other information
Question -> when we pass some file which doesn't exist ex -c webpack.testconfig.js we fallback to default config webpack.config.js, should we allow that considering the user already passed a config they want to use or should we throw err that config not found?

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

I don|t think this is correct. We need interpret to take both cjs and regular files. This takes a file and its extention, the double filtering is to check for cjs ext

@evenstensberg
Copy link
Member

Considering a file is a small set, I think this is ok

@evenstensberg evenstensberg reopened this Aug 15, 2020
@anshumanv anshumanv marked this pull request as ready for review August 15, 2020 16:53
@anshumanv anshumanv requested a review from a team as a code owner August 15, 2020 16:53
snitin315
snitin315 previously approved these changes Aug 16, 2020
@anshumanv
Copy link
Member Author

This is how gulp also uses it, prepares env which need specific module, rest fallback to the default node loader, no use of filtering

evenstensberg
evenstensberg previously approved these changes Aug 17, 2020
Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

I think this removes cjs support, could you make a test file first?

@anshumanv
Copy link
Member Author

I think this removes cjs support, could you make a test file first?

Nope doesn't remove, I already added tests for cjs when I added it's support #1727

@webpack-bot
Copy link

@anshumanv Thanks for your update.

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

@evenstensberg Please review the new changes.

@alexander-akait
Copy link
Member

Question -> when we pass some file which doesn't exist ex -c webpack.testconfig.js we fallback to default config webpack.config.js, should we allow that considering the user already passed a config they want to use or should we throw err that config not found?

I think without extensions we should confider it as a JS file, it is edge case, but it doesn't mean we should ignore it

@anshumanv
Copy link
Member Author

I think without extensions we should confider it as a JS file, it is edge case, but it doesn't mean we should ignore it

@evilebottnawi talking about config file that doesn't exist eg webpack.doesntexist.js, should we throw error or fallback to default config?

@alexander-akait
Copy link
Member

@anshumanv if config doesn't exists we should throw an error

@anshumanv
Copy link
Member Author

@anshumanv if config doesn't exists we should throw an error

Gotcha, will implement in near future

@anshumanv
Copy link
Member Author

I think we're good here then

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @webpack/cli-team

@alexander-akait alexander-akait merged commit b071623 into webpack:next Aug 19, 2020
@anshumanv anshumanv deleted the rechoir-default branch August 19, 2020 16:14
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