-
Notifications
You must be signed in to change notification settings - Fork 27
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
Bugfix for error messages not showing in mc-build
#2189
Conversation
🦋 Changeset detectedLatest commit: 8d71c3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/12NcYRpaQGEA5VR2CADypVyxLtv7 |
messages = formatWebpackMessages({ | ||
errors: rawMessages.errors.map((e) => e.message), | ||
warnings: rawMessages.warnings.map((e) => e.message), | ||
}); |
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.
Cool approach. Thinking about it "out load":
Do you think it would be possible to "sniff" the format. So see if we need to map the errors? Should we actually do that? Wondering what happens when react-dev-utils
gets fixed 🤔.
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.
We probably could with some inspection on fields of the stats.toJson
result. The only thing i think is that when react-dev-utils
does move to webpack 5, it's likely going to be part of a very large update anyway.
Another proposal, since it's one tiny function, could be to pull formatWebpackMessages
into this repo and support webpack 5 out of the box?
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.
Interestingly, they have a PR open to fix it on their side with the detection, it looks like it won't break this fix:
https://github.com/facebook/create-react-app/pull/10098/files
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.
Interesting. Seems like it would continue to work. Maybe you can add a //TODO: Remove after 'react-dev-utils' natively supports webpack@5
to your code so we don't forget :)
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.
Done 😄
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.
Nice. Thanks 💯. Totally fine by me given it's temporary and actually solves a quite annoying issue.
Waiting for @emmenko to also confirm.
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 thanks for finding a workaround!
Summary
React-dev-utils doesn't support the webpack 5 stats, so this is an adapter to allow errors to propogate properly to
mc-scripts build
.Description
Resolves #2188