-
-
Notifications
You must be signed in to change notification settings - Fork 622
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
migrate basic group #1837
migrate basic group #1837
Conversation
} | ||
}); | ||
} | ||
return configPathExists ? predefinedConfigPath : configPath; |
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.
To be honestly it is wrong, webpack uses resolver to resolve js
/ts
/cjs
/mjs
, even css
/scss
/less
and other extensions, I think It should be removed
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 think all test will be passed without it
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.
makes sense, let's simply do path.resolve
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.
Ah so we don't even need to do path.resolve since core does this already for relative paths, we just need to pass the entry
@evilebottnawi PTAL if this is what you had in mind |
CI broken with webpack@next 😞 |
@snitin315 can you test the issue with this branch now? |
Yes, because we need remove all unnecessary stuff, there are a lot of problems with this due bad code from history can you look at the problem? |
We need check bug from webpack-cli or regression from webpack |
@evilebottnawi I think regression in webpack rc-1, works fine with rc-0 |
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.
Can you add a test case for a non-js entry point too?
Done |
ac1ab7f
to
50399f1
Compare
/cc @webpack/cli-team |
if (existsSync(rootIndexPath)) { | ||
defaultEntry = './index.js'; | ||
} else { | ||
defaultEntry = './src/index.js'; |
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.
Do we have tests for both of the default entries i.e, ./index.js
and ./src/index.js
?
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.
This should be solved on webpack side, here invalid solution, because index can have cjs
/ts
/mjs
and etc extensions
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.
Need to change a bunch of tests again because of current behaviour, let's fix this separately? Here's the issue for any discussion - #1852
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.
/cc @webpack/cli-team
@anshumanv Thanks for your update. I labeled the Pull Request so reviewers will review it again. @evilebottnawi Please review the new changes. |
What kind of change does this PR introduce?
refactor
Did you add tests for your changes?
Yeaaa
If relevant, did you update the documentation?
No need
Summary
Does this PR introduce a breaking change?
Nayyy
Other information