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

regression: xo: false in nested package.json is not supported any more #741

Open
JounQin opened this issue Nov 20, 2023 · 9 comments
Open

Comments

@JounQin
Copy link

JounQin commented Nov 20, 2023

xo: false means ignoring the files under the package.json file previously, but it stops working in recent versions.

@spence-s
Copy link
Contributor

spence-s commented Nov 20, 2023

Could you describe the scenario better? I don't think this fix is what we want to do.

I take it that you are in a mono-repo, you have a xo installed in a nested package, and your xo config is at the root level in your package.json.

.
├── packages
   └── some-package
         |------- package.json (xo installed here, but config is 2 dirs up)
|- package.json (xo config here)

You should instead do something like
from your nested package folder:
npx xo --cwd ../..

This will fix your problem I think, it's a common trip up for people with mono-repos.

@JounQin
Copy link
Author

JounQin commented Nov 21, 2023

npx xo --cwd ../..

It seems too verbose to me personally, and I didn't know it would work. Why not fix like this?

@JounQin
Copy link
Author

JounQin commented Nov 21, 2023

OK, I found out that it's actually specifically disabled at https://github.com/rehypejs/rehype-github/blob/7d7e851b751413c1ec95e3fbdf1c48e4b71155e5/packages/break/package.json#L37, however in this case, xo should just ignore linting actually or change to use the root config like the current fix.

I would prefer not linting at all because xo is specifically disabled.

@JounQin
Copy link
Author

JounQin commented Nov 21, 2023

Because previously xo used xo: false for nested packages, to continue looking for a parent package.json.
Omitting xo: false, previously, resulted in the defaults being applied.
Which is exactly what xo is now doing when xo: false is used.

from @wooorm at rehypejs/rehype-github#2 (comment)


I think I'll change the fix by check whether xo is false instead. Do you think this is a regression? @spence-s

@JounQin JounQin changed the title wrong nested package.json resolved regression: xo: false in nested package.json is not supported any more Nov 21, 2023
@spence-s
Copy link
Contributor

At some point, years ago, xo would search all the way up to the home directory for a "global" xo config. But this in itself caused lots of confusion and bugs for many users and was hard to integrate with eslint properly. (ie, couldn't lookup plugins and configs correctly for configs outside of the repo)...

This behavior was specifically removed in favor of requiring configs to live in the repos, so this is not a bug nor a regression, but is working as intended and stopping search at cwd as intended.

Does it still limit people with mono-repos? Yes it does limit them a bit and they may have to "hack" cwd like I showed above.

Can you still use xo with mono repos using the recommended approach? absolutely, xo works just fine in mono-repos if set up correctly.

Could we improve this or accept functionality that improves this UX? That would be up to @sindresorhus, but probably would be willing to change things slightly if there was a good argument for it.

However, it looks to me that xo is just being used in a mono repo set up here in a way that doesn't play nicely with its design. I would recommend refactoring the original repo per recommended mono repo setup.

TL;DR: not a bug nor regression, nor do the PRs that keep searching up something we want to add at this time. If/when we move to the the new config style, we may reconsider if that makes solving resolution bugs simpler.

@JounQin
Copy link
Author

JounQin commented Nov 21, 2023

OK, then it could be feature request then? xo: false to disable xo's functionality for nested package.json.

@spence-s
Copy link
Contributor

spence-s commented Nov 21, 2023

I would personally not support this behavior change.

You can already get the behavior you want from the way it is right now.

A pr for this would need to be comprehensive and have a lot of tests to account for resolving plugins and configs potentially outside of the repo. So maybe it would be considered if you decided to take on this relatively large amount of work.

Note: if you have an xo field - even if it is "false" in the package.json, search will prematurely stop there. You need to remove the xo field entirely from package.json to have it continue searching up to the cwd you set.

@wooorm
Copy link

wooorm commented Nov 21, 2023

xo: false was recommended before. I don’t think this feature was removed that many years ago (e0f81a7).
I’m pretty sure we’ve used it in many places, since it was added 7 years ago, up until maybe a year ago? 50 or so?

Anyway, I’m fine not supporting this.

@spence-s
Copy link
Contributor

The PR you linked was from 3.5 years ago and it does look like the change happened there.

It also includes the documentation to simply omit the "xo" field in order to continue searching up to the cwd, changed from the previously recommended xo: false

I would support this change to respect xo: false as long as it definitely stops searching at the cwd (like it does now), which would be a very minor behavior change.

However, its probably easier to just remove the xo: false fields from your package.jsons

as with all changes the final say is up to @sindresorhus not me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants