bcomnes / sublime-standard-format

:sparkles: Runs standard --fix against the javascript in your ST3 window on save or manually.
https://packagecontrol.io/packages/StandardFormat
MIT License
60 stars 21 forks source link

Saving file with standard js error leads to content being overwritten by error message #54

Closed ErikWittern closed 4 years ago

ErikWittern commented 7 years ago

Since a few days, when I save a file that contains a standard js error, the content of that file will be overwritten with the error message produced by standard js. This behavior does only appear if I have StandardFormat enabled - disabling the plugin resolves the problem. I attach two screenshots to illustrate the behavior.

I am using version 5.0.0 of StandardFormat, and have Sublime Text Build 3126.

Maybe this has to do with my specific setup? I am of course happy to report further details if needed.

Code with a standard js error: screen shot 2016-11-30 at 9 48 25 am

Saving the above file leads to: screen shot 2016-11-30 at 9 48 38 am

bcomnes commented 7 years ago

Make sure you are running the latest standard module. Can you confirm that? Older versions didn't support stdin formatting and just return the listing errors. I have some major improvements in the pipeline coming soon as well.

Either install it in global or locally.

ErikWittern commented 7 years ago

I checked: I had version 8.3.0 of standard installed. I just upgraded (globally) to 8.6.0. Unfortunately, the error persists, even with the new (latest) version.

bcomnes commented 7 years ago

Unfortunately the plugin doesn't provide much feedback on what its doing at the moment. The only thing I can think of would be that you have two versions of standard in your path the the plugin is putting together.

Some things to check:

npm uninstall -g standard

if you use nvm, this gets complicated because standard could still be living in a different npm location that nvm uses, and the plugin blindly uses. You might double check /usr/local/lib/node_modues

I would also double check to explicitly update the local standard in the project by updating the package.json and rm -rf node_modules and reinstall everything.

In my experience, this issue stems from the plugin using the wrong version of of the formatter command, or finding the wrong copy of it at the wrong version.

Again, this is way more painful than it needs to be and I hope to have a much improved and introspective version of the plugin out soon this week. I've been swamped at work though and have had limited time to fix it.

Worse case scenario, you might have to disable the plugin for a day or two ☹️ Super duper sorry.

ErikWittern commented 7 years ago

Thank you for all the feedback!

Uninstalling standard (npm uninstall -g standard) and reinstalling it did not help. I do use nvm, though, so maybe the problem is caused by other versions of standard being installed somewhere...

The problem actually appears across projects, so I assume that deleting a node_modules folder won't help much.

I might try to figure out if nvm plays a role, but I can also wait for an update. The plugin is great, I appreciate all the good work you are doing!!!

bcomnes commented 7 years ago

I just updated the plugin to 6.0.1. Once it gets released, let me know if this problem still persists.

xavdid commented 7 years ago

Hey there, getting this issue with plugin version 6.0.2 and standard version 8.6.0. I've got nvm, but I confirmed that all 3 installs are using the most up-to-date standard.

It's interesting- running cat utils.js | standard --stdin --fix on the command line outputs both the fixed js file and the rest of the remaining errors:

const responsePromise = z.request({
  url: 'https://api.steampowered.com/IPlayerService/GetOwnedGames/v0001?include_appinfo=1',
  params: {
    tag: bundle.inputData.tagName
  }
})
standard: Use JavaScript Standard Style (http://standardjs.com)
standard:   <text>:2:7: 'responsePromise' is assigned a value but never used.
standard:   <text>:2:25: 'z' is not defined.
standard:   <text>:5:10: 'bundle' is not defined.

Without the fix flag, I just get the errors (no formatted file).

It seems like the fix flag isn't being run by the command, which is weird because in the ST console it shows it: command: ['standard', '--stdin', '--fix'].

Anyway, let me know if there's anything else I can help with!

bcomnes commented 7 years ago

I think the linting errors get printed when the formatter fails, but I'm still trying to understand the behavior of --fix flag. I've seen this when there are major syntax errors in the source code and the parser craps out. Kinda weird. This might require upstream fixes.

Do have some source code that causes the issue that you can gist for me?

796F commented 7 years ago

Actually same issue for me.

using plugin 6.1.0 image

$ standard --version
9.0.1

Interestingly, my command line output is also broken, but when i run standard --fix it does it correctly on my project. just not on save file

bcomnes commented 7 years ago

@xiamike Which OS?

bcomnes commented 7 years ago

@xiamike also set "log_errors": true in your sublime standard format config and maybe we can find some hints there.

796F commented 7 years ago

macOS Sierra, Version 10.12.3 (16D32)

also, strangely working now...
when I run npm install standard it installs v7.0.1 to my project node_modules when I run npm install standard -g it installs v9.1.0 globally.

I uninstalled an older version of standard which was in node_modules, and reinstalled the global one, restarted sublime and it's working now.

Not sure if that's helpful for others?

bcomnes commented 7 years ago

The plugin defaults to local dependencies first, if it finds them. It falls back to global dependencies if they are present as well.

Works well with 8 and 9 as far as I am aware. I think 7 works too.

796F commented 7 years ago

@bcomnes i reinstalled the old module, same error appears, but log_errors doesn't show anything new in the output.

Actually I'm almost certain it's v7.1.2 standard that's in project/node_modules now. When I again run npm install standard the issue pops up, which makes sense if it defaults to the local dep.

With the older version there, again overwrites my file with

  <text>:1:1: 'importScripts' is not defined.
  <text>:4:20: 'Pusher' is not defined.
  <text>:6:1: 'self' is not defined.
  <text>:15:2: Mixed spaces and tabs.
  ...

removing it and letting the module find global standard which is v9.0.1 fixes this for me

bcomnes commented 7 years ago

I would say update to the latest standard then in the project! Bump the version in package.json and reinstall the project Deps. I should probably add a check on the version number. We already parse it out at https://github.com/bcomnes/sublime-standard-format/blob/master/standard-format.py#L105-L108

kldavis4 commented 7 years ago

I am running into this issue now as well after recently upgrading standard to 10.0.2 installed within the project. Any ideas how to troubleshoot further?

bcomnes commented 7 years ago

What os? Check out the default preferences for some ways to turn on more verbose logging.

bcomnes commented 7 years ago

Please describe the symptoms and check the sublime console for errors.

kldavis4 commented 7 years ago

OSX 10.12.4

I can run standard from the command line: node_modules/.bin/standard --fix index.js

and I get this warning on stdout:

The react/jsx-space-before-closing rule is deprecated. Please use the react/jsx-tag-spacing rule with the "beforeSelfClosing" option instead.

If enable StandardFormat and run it on my file in SublimeLinter it inserts the warning message above at the top of the file being modified.

I don't see anything that looks like it is coming from StandardFormat when log_errors is true. If I install standard globally and set the PATH to the installed bin directory, the issue goes away.

Here is the output I see at startup when I set the PATH:

StandardFormat:
  global_path: /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
  search_path: /Users/kelly/.nvm/versions/node/v7.9.0/bin:/Users/kelly/projects/kuali/cor/cor-workflows-prettier/node_modules/.bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
  found standard at /Users/kelly/.nvm/versions/node/v7.9.0/bin/standard
  command: ['standard', '--stdin', '--fix']

and here is the output when I don't set the PATH:

StandardFormat:
  global_path: /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
  search_path: /Users/kelly/projects/foo/cor/bar/node_modules/.bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
  found standard at /Users/kelly/projects/foo/cor/bar/node_modules/.bin/standard
  command: ['standard', '--stdin', '--fix']

I have confirmed that both standard binaries are the same version (10.0.2)


~/.nvm/versions/node/v7.9.0/bin/standard --version
10.0.2
./node_modules/.bin/standard --version
10.0.2
nikoladev commented 7 years ago

@kldavis4 I had the exact same error and I think I've found the source. It seems the error is in node_modules/eslint-config-standard-jsx/eslintrc.json and it's fixed in this commit.

That fix still hasn't made it into a release, and I don't know when it will, but if you manually apply the fix from that commit to node_modules/eslint-config-standard-jsx/eslintrc.json the bug should be fixed. And when the new release does come out it will simply overwrite any changes you've manually made (if I'm not mistaken) and the fix will be applied permanently.

Let me know if it works!

bcomnes commented 7 years ago

FWIW, every time I've been able to re-create this issue is when an old version of standard is used to format the code. If someone can point me to a file in a gist or repo hash, that is reproducible I can dig into this more. I think maybe checking the version of standard every time would also help mitigate this issue.

kldavis4 commented 7 years ago

@nikoladev that just suppresses the warning, though, right? It doesn't fix issue with the error message going into the file being formatted, does it?

@bcomnes any thoughts on the fact that using global standard binary seems to work but the node_modules binary causes the warning to be inserted? (besides version mismatch)

bcomnes commented 7 years ago

From what I've come across, when the buffer gets replaced by an error message, its because someone tried to use an old standard that didn't have the correct --fix API that this plugin expects. Maybe try setting "check_version": true, so that you get the version from the perspective of sublime. Its weird that it looks like you are running standard 10, but still getting this issue.

Perhaps standard could be changed to improve this API so that this doesn't happen ever, but in terms of what we can do from this plugin is not make version checking optional and prevent replacing the buffer if standard that is found doesn't meet the minimum version requirement.

EDIT whoops hit the wrong button, didn't mean to close.

kldavis4 commented 7 years ago

Could there be a (peer) dependency in standard that might be different when running from global versus node_modules? Maybe something that would break --fix. I tried adding standard to react-starter-kit and formatting and didn't have the issue regardless of whether i was using the global version of standard.

bcomnes commented 7 years ago

Gosh, I hope not. The way the plugin works is it walks up to the root fs from the open file or project folder, looking for node_modules/.bin folders to supplement the PATH with when searching for the standard bin. Like I said though, turn on the check version setting in the plugin settings and see what version it reports.

kldavis4 commented 7 years ago

Same version either way: standard version: ('10.0.2\n', b'')

bcomnes commented 7 years ago

Can you gist a sample buffer that triggers the issue?

kldavis4 commented 7 years ago

You mean a file where it occurs? This one-liner will do it: export default () => {}

bcomnes commented 7 years ago

Very strange. Should work!

sf

No idea why thats not working for you. Try in an empty project with just standard to see if its specific too your global environment or just the project. And if its specific to the project, what recipe of dependencies are causing the issue.

kldavis4 commented 7 years ago

yeah, I am beginning to think it is something to do with the project dependencies. I'll see if I can narrow it down and report back.

bcomnes commented 7 years ago

Sorry you are running into issues. If you do find a situation that triggers this, I would be happy to try to fix.

kldavis4 commented 7 years ago

thanks for your help in running it down. It sounds like from what you are saying that there is a problem with standard --fix right (which in my case might be due to some funky dependencies)? should I be able to repro this using just the command line?

bcomnes commented 7 years ago

Hard to say.

nikoladev commented 7 years ago

@kldavis4 Yeah, it actually stops the insertion of that line you quoted:

The react/jsx-space-before-closing rule is deprecated. Please use the react/jsx-tag-spacing rule with the "beforeSelfClosing" option instead.

Also, I share your suspicions about project dependencies having something to do with it. I'll do some more testing and I'll see if I can figure out something.

nikoladev commented 7 years ago

@bcomnes The bug with the inserted text at the top of the file can be reproduced by adding standard as a devDependency to a fresh create-react-app build. This method isn't very precise at determining which exact dependencies cause the bug (because create-react-app has A LOT of dependencies), but hopefully it will help a little bit.

Here's a quick way to reproduce the bug.

Create a new folder and in it run:

$ npm install create-react-app
$ node_modules/.bin/create-react-app test
$ cd test

Opening src/App.js and doing a Format current file works fine. Now add standard:

$ npm install standard --save-dev

If you go back to src/App.js and format it again, this time the following will be added to the top of the file:

The react/jsx-space-before-closing rule is deprecated. Please use the react/jsx-tag-spacing rule with the "beforeSelfClosing" option instead.

nikoladev commented 7 years ago

Version 10.0.3 of standard has fixed this issue. At least for me :)

bcomnes commented 4 years ago

Should be fixed.