Skip to content

Order lint warnings by most recently edited file (#2378)#2409

Closed
kombucha wants to merge 2 commits into
react:mainfrom
kombucha:2378-lint-warning-most-recent-file
Closed

Order lint warnings by most recently edited file (#2378)#2409
kombucha wants to merge 2 commits into
react:mainfrom
kombucha:2378-lint-warning-most-recent-file

Conversation

@kombucha

@kombucha kombucha commented May 29, 2017

Copy link
Copy Markdown

Hi
I've added sorting to the warning messages by last file modified time. This makes recently edited file pop up at the top of the error list. This affects logs both in the terminal and in the browser console (which seems more intuitive anyway).

I've checked by running npm-start in the project and modifying templates/index.js and templates/App.js alternatively, verifying that the order is coherent (see screenshots).

screen shot 2017-05-29 at 23 42 08

And then after editing the index.js file

screen shot 2017-05-29 at 23 42 31

I've also checked in the browser console of course. I'm working on a mac.
If this seems like the proper resolution for this bug, I can check on more platforms (like windows) !

Cheers

@facebook-github-bot

Copy link
Copy Markdown

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon

gaearon commented May 30, 2017

Copy link
Copy Markdown
Contributor

Wow, that’s really neat! I’m going to review this when I get some extra time (currently busy with other things).
Or maybe somebody else can.

Thanks for PR!

@ulrikstrid

ulrikstrid commented Jun 27, 2017

Copy link
Copy Markdown
Contributor

When looking at the console I want to see the most important thing last. So for me it would be more useful to have the most recent file last.

@gaearon

gaearon commented Jun 28, 2017

Copy link
Copy Markdown
Contributor

When looking at the console I want to see the most important thing last

Do you mean browser console? I think in the browser console we might as well hide anything except last edited file.

...compilation.warnings,
].sort((warning1, warning2) => {
if (!warning1._lastModifiedDate) {
warning1._lastModifiedDate = fs.statSync(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a way to avoid these calls every time? I think webpack may expose this information in a cached way via its own "filesystem" object. Or maybe there's some other global object that already contains last edit times. Could you check for this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'll take a look

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I now use compilation.fileTimestamps. I still have fs.statSync as a fallback because fileTimestamps is empty on the first compile for some reason :/. But the next few compiles are using webpack's stats.

@gaearon gaearon modified the milestones: 1.0.x, 1.0.8 Jun 28, 2017
@kombucha

Copy link
Copy Markdown
Author

I understand your point @ulrikstrid : if you have warnings in a lot of files, you might scroll past the first file in the terminal, so having the order reversed might be more practical.
If maintainers agree, I can change this easily :)

@ulrikstrid

Copy link
Copy Markdown
Contributor

Yeah, I ment the terminal and not the browser.

@svicalifornia

svicalifornia commented Aug 16, 2017

Copy link
Copy Markdown

It would be nice if there was some way to specify an option for sort direction somewhere.

(More generally, it would be nice if create-react-app/react-scripts had a config file to specify options of many kinds. It would likely reduce many folks' need to eject and allow them to continue benefitting from new developments in CRA. If such a config file already exists, please tell me.)

@gaearon

gaearon commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

Every option doubles the amount of things that can go wrong and that we need to test for. We intentionally limit how much is customisable to keep the project manageable and stable. I encourage you to look into alternatives like Neutrino if you prefer to customise things.

@gaearon gaearon modified the milestones: 1.0.x, 2.0.x, 2.x Jan 14, 2018
@iansu iansu modified the milestones: 2.x, 3.x Mar 10, 2019
compiler.plugin('after-compile', (compilation, callback) => {
// Order compilation warnings by most recently modified
compilation.warnings = [
...compilation.warnings,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

since it sorts in place, is it necessary to clone the array?

Comment on lines +146 to +154
].sort((warning1, warning2) => {
const warning1Time = compilation.fileTimestamps[
warning1.module.resource
] || fs.statSync(warning1.module.resource).mtime.getTime();
const warning2Time = compilation.fileTimestamps[
warning2.module.resource
] || fs.statSync(warning2.module.resource).mtime.getTime();

return warning1Time < warning2Time ? 1 : -1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
].sort((warning1, warning2) => {
const warning1Time = compilation.fileTimestamps[
warning1.module.resource
] || fs.statSync(warning1.module.resource).mtime.getTime();
const warning2Time = compilation.fileTimestamps[
warning2.module.resource
] || fs.statSync(warning2.module.resource).mtime.getTime();
return warning1Time < warning2Time ? 1 : -1;
].sort((...warnings) => {
const [a, b] = warnings.map(({ module: { resource } }) => (
compilation.fileTimestamps[resource] || fs.statSync(resource).mtime.getTime()
));
return a === b ? 0 : a < b ? 1 : -1;

@kombucha

Copy link
Copy Markdown
Author

tenor-112513283

@kombucha kombucha closed this Jun 25, 2023
@kombucha kombucha deleted the 2378-lint-warning-most-recent-file branch June 25, 2023 20:29
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.

7 participants