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

feat: update Webpack config #7638

Closed
wants to merge 22 commits into from
Closed

feat: update Webpack config #7638

wants to merge 22 commits into from

Conversation

yordis
Copy link

@yordis yordis commented Sep 4, 2019

I just fixed the requested changes from #7450 since I would love to move forward with this, I can help to make it to master

cc: @iansu @petetnt @sokra @ianschmitz @mrmckeb @TheLarkInn

@yordis
Copy link
Author

yordis commented Sep 4, 2019

Fixed merge conflicts!

@yordis
Copy link
Author

yordis commented Sep 4, 2019

I need some help with the CI errors, I don't even know how to get to the error report from the CI

@petetnt
Copy link
Contributor

petetnt commented Sep 5, 2019

You should be able to see the errors in https://dev.azure.com/facebook/create-react-app/_build/results?buildId=356&view=logs&jobId=8999f565-280a-527f-721e-375d49cc4cd5

@heyimalex
Copy link
Contributor

My biggest issue is that it's already annoying tracking blames back across the merge that happened in #5722, so I'm kinda against re-separating them. Our webpack config is big and ugly, but most of it is essential complexity and no one is ever going to be totally happy or comfortable with it. Refactoring a big mess into two piles maybe feels good, but sometimes it just gives you two big messes you have to reason across.

All that being said, on the face the code does look a lot nicer! Maybe it would be worth it.

@yordis
Copy link
Author

yordis commented Sep 13, 2019

@heyimalex I understand where you are coming from but worth to point out that the issue before that PR is that we had two configurations sharing nothing between them. Therefore, you had to update the configs that were shared in both files.

I think we went to the other extreme where everything is one file.

Using webpack-merge people could split what really goes into production or development mode.

I believe that this is the common ground between the two worlds.

@yordis
Copy link
Author

yordis commented Sep 13, 2019

Hey folks, I would like your help with the failing situation since it related to eslint package

Error: Failed to load plugin 'import' declared in 'BaseConfig » /home/vsts/work/1/yarn-cache/v4/npm-eslint-config-react-app-5.0.2-alpha.0-676ad4e98b055498e0bbed90c651f736f9ba322f/node_modules/eslint-config-react-app/index.js': You cannot require a package ("eslint-plugin-import") that is not declared in your dependencies (via "/home/vsts/work/_temp/tmp.Xq1mcftxoc/test-app-pnp/__placeholder__.js")

@stale
Copy link

stale bot commented Oct 13, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Oct 13, 2019
@yordis
Copy link
Author

yordis commented Oct 13, 2019

.

@stale stale bot removed the stale label Oct 13, 2019
@mrmckeb mrmckeb added this to the 3.3 milestone Oct 17, 2019
@mrmckeb mrmckeb changed the title Feature/organize config feat: update Webpack config Oct 17, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 17, 2019

Thanks @yordis - before doing anything else, let's hear from @iansu and/or @ianschmitz.

I think this is a candidate for 3.3.

@yordis
Copy link
Author

yordis commented Oct 17, 2019

@mrmckeb just let me know, I am here to go back actively to fix the existing problems

@yordis yordis requested a review from amyrlam as a code owner October 25, 2019 02:42
@andriijas
Copy link
Contributor

@heyimalex I think you are on to something. Webpack 5 is moving towards even more community praised default configs so we are going to be able to clean up a bit. If we are going to start separate configs I would want to apply the same kind of thinking as we do with UI components. More lego blocks/small individual configs with separate tests to build up the full configuration.

@yordis
Copy link
Author

yordis commented Nov 13, 2019

More lego blocks/small individual configs with separate tests to build up the full configuration.

@andriijas if you go for that route,

I hope you understand how problematic JavaScript plain objects are, and leverage #7866 in terms of webpack-chain.

Regardless,

let me know what you would like to do so I clean this up 😄

@ianschmitz ianschmitz modified the milestones: 3.3, 3.4 Dec 5, 2019
@iansu iansu modified the milestones: 3.4, 3.5 Feb 14, 2020
@yordis yordis closed this Dec 23, 2020
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.

9 participants