AtomLinter / linter-eslint-node

ESLint plugin for Atom/Pulsar Linter (v8 and above)
https://web.pulsar-edit.dev/packages/linter-eslint-node
MIT License
4 stars 3 forks source link

The editor goes blank when fix when save is enabled #24

Open eaviles opened 2 years ago

eaviles commented 2 years ago

Issue Type

Bug

Issue Description

Whenever I have "Fix errors on save" enabled, the entire editor window goes blank before appearing again with the corrected file. The editor stays blank for about 1 second and when is back the current cursor and scroll position is reset which makes the option unusable.

Bug Checklist

Atom version: 1.63.0-nightly1
linter-eslint-node version: 1.0.5
Worker using Node at path: /Users/edgardo/.nvm/versions/node/v16.14.2/bin/node
Worker Node version: v16.14.2
Worker PID: 1591
ESLint version: 8.14.0
ESLint location: /Users/edgardo/Developer/cradl-gdt/node_modules/eslint/
Linting in this project performed by: linter-eslint-node
Hours since last Atom restart: 0.2
Platform: darwin
Current file's scopes: [
  "source.js"
]
linter-eslint-node configuration: {
  "autofix": {
    "fixOnSave": true,
    "rulesToDisableWhileFixing": [],
    "ignoreFixableRulesWhileTyping": false
  },
  "scopes": [
    "source.js",
    "source.jsx",
    "source.js.jsx",
    "source.flow",
    "source.babel",
    "source.js-semantic",
    "source.ts"
  ],
  "nodeBin": "node",
  "warnAboutOldEslint": true,
  "disabling": {
    "disableWhenNoEslintConfig": true,
    "rulesToSilenceWhileTyping": []
  },
  "advanced": {
    "disableEslintIgnore": false,
    "showRuleIdInMessage": true,
    "useCache": true
  }
}
savetheclocktower commented 2 years ago

To diagnose this, I'd like to ask a couple questions:

  1. Forgive me, but it's worth ruling this out up front: is it possible that you're using a toolchain that automatically fixes files when they're saved? I ask because I encountered this exact problem on one of the projects I contribute to at work, and that was the culprit. If you're unsure, uncheck “Fix on Save” and introduce a fixable error, then save and see if the file updates. If so, you can disable “Fix on Save” for just that project by using the .linter-eslint file described in the README.
  2. Failing that: does this happen on all files, or only files larger than a certain size?
  3. Try this: disable “Fix on Save,” introduce a fixable error, save it in the editor, then run npx eslint --fix path/to/file.js and watch the contents of the file in Atom's editor pane. Do you see similar symptoms (i.e., a blank editor window for a moment)?

If none of the above explains the issue, then I'll do my best to track this down. It's odd because, to the best of my understanding, the fix-on-save behavior has not substantially changed in this package from what it was in linter-eslint.

UziTech commented 2 years ago

I did notice this is on Atom v1.63.0-nightly1 which has an updated version of electron.

savetheclocktower commented 2 years ago

@UziTech unrelatedly, 1.0.6 isn't showing up when I check for updates, and the page on atom.io gives me a 500 :-/

UziTech commented 2 years ago

@savetheclocktower ya atom is having issues with the website. Unfortunately I don't have access to do anything about it. I think the latest version is part of the same problem.

dpkrowe commented 2 years ago

I have noticed this being an issue as well, but only on files with several thousand lines of code. Smaller documents have not (yet) had an issue. I am on the latest version of atom, 1.60.0 For me, the text area inside of atom goes blank for 1-2 seconds before re-appearing at the first line. This is annoying since I now have to go back to the line I was working on before the save, but it still beats manually fixing all lint errors.

eaviles commented 2 years ago

@savetheclocktower:

1) this is a new project with no toolchain, no git hooks, no watchers, I only have ESLint configured (plain .eslintrc file). Unchecking fix on save then saving a file with fixable issues doesn't make the file reload.

2) happens with any file covered by ESLint, size doesn't seem to be an issue. I tried with several of the files in the project and most of them make the editor reload independently of the file size (100 lines, 500 lines etc.) curiously, one of my files with 500 lines doesn't make it reload but a 300 lines one would.

3) I disabled "fix on save", changed the file to have issues, then run the ESLint CLI to fix it and the Atom editor picked up the changes without reloading the window.

Happy to run more scenarios to help diagnose. Let me know.

savetheclocktower commented 2 years ago

Thanks for the information, @eaviles. This is stumping me so far.

We are using the same approach for fixes that linter-eslint uses, which is to let the ESLint engine handle fixing and writing the new file. The way we communicate with the worker is slightly different, but I don't see how that matters. I haven't gone digging into the ESLint source code, but I would assume that the file is being rewritten atomically.

If the file is being written atomically, then it stands to reason that Atom isn't picking up on the filesystem change too quickly — that once it realizes the files has changed, the new file contents are there to consume. If so, then the momentarily blank editor pane would suggest a bug in Atom somewhere, but I'm not sure of the form it would take — or, in @dpkrowe's case, why it would manifest only some of the time.

There's one possible way to sidestep this: we could rely on ESLint to make the fixes, but to hand the new file to us as a string rather than save it to disk. We could then replace the entire contents of the file within Atom and perform a second save. This feels silly, but if this ends up being a widespread issue, it'd almost certainly be worth doing.

Leaving this open for now so that other people can weigh in with their experiences. Maybe we can narrow down the conditions under which it happens.

cmens commented 2 years ago

Hi,

Any progress on this? I'm having the same issue. It only happens on large files, around 800+ lines. Here is my Linter Eslint Node: Debug output:

Atom version: 1.58.0 linter-eslint-node version: 1.0.5 Worker using Node at path: /usr/bin/node Worker Node version: v16.15.0 Worker PID: 26172 ESLint version: 8.8.0 ESLint location: /home///**/node_modules/eslint/ Linting in this project performed by: linter-eslint-node Hours since last Atom restart: 0.4 Platform: linux Current file's scopes: [ "source.js" ] linter-eslint-node configuration: { "autofix": { "fixOnSave": true, "rulesToDisableWhileFixing": [], "ignoreFixableRulesWhileTyping": false }, "scopes": [ "source.js", "source.jsx", "source.js.jsx", "source.flow", "source.babel", "source.js-semantic", "source.ts" ], "nodeBin": "node", "warnAboutOldEslint": true, "disabling": { "disableWhenNoEslintConfig": true, "rulesToSilenceWhileTyping": [] }, "advanced": { "disableEslintIgnore": false, "showRuleIdInMessage": true, "useCache": true } }

scagood commented 2 years ago

@savetheclocktower do you think that this could be a side effect of removing the queue?

That being that the process if running multiple tasks at once it just can't write the fixes fast enough to the file system?

savetheclocktower commented 2 years ago

@scagood I think that there's still an implicit queueing based on how ndjson works — there's one worker per project, and as far as I know it's synchronously linting/fixing each file in turn if a bunch of them request to be linted at the same time. Only about 90% certain of that, though, so if you showed me evidence otherwise, I wouldn't accuse you of lies.

I haven't had the time to try a serious stress test, but if nobody else figures it out eventually I'm just going to lock myself in a room with this bug and have it out.

scagood commented 2 years ago

Just for a bit of an explanation from my thought process, the promise queue I added initially was used for queueing jobs, not for queing the json chunks (the chunks queueing was actually done by the Stream#write call). index.js:148-152

So before the jobs would get stalled like this (stalling is done it atom, and is highlighted in red): with-the-queue

And this is what it is now: without-the-queue

I think the way I would start to attempt to test this hypothesis would be by recording the start time and the end time of the jobs. Then storing that in a way that we can then include in the debug output. What do you think?

savetheclocktower commented 2 years ago

Yeah, I get the distinction, and I think I was just mistaken. My faulty recollection was that methods like ESLint.outputFixes were synchronous, but of course they're async, and the jobs are probably interleaved in the way you're talking about.

That may or may not be causing us problems, but it sounds like this bug is happening when someone is saving a single file, not just in the midst of a bunch of linting jobs. I really don't want to go digging into the ESLint internals to figure out exactly how it's writing files and whether the approach is different than it used to be, but I might have to.

scagood commented 2 years ago

mmm, you're right, I have also had this happen when saving singular files. I am just trying to come up with some metric to help work this out 😅

Interestingly I have only had this when working on linux (ubuntu) but never when I am on mac, the main difference I can think of (excluding the os ofcs) is when I am on linux I have a load more plugins loaded 🤔

savetheclocktower commented 2 years ago

Maybe the specifics of the filesystem are the difference? Maybe it's the speed of the hard drive?

scagood commented 1 year ago

I have not seen this issue in a very long time.

@eaviles @UziTech @dpkrowe @cmens have any of you seen this recently, if not, can we close this issue?

@savetheclocktower I dont think you experienced this, right?

savetheclocktower commented 1 year ago

I never experienced this. Let's see if other people weigh in. If nobody has had this happen in a while, then we can close this ticket.