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

Change tools, refactor package.json #101

Closed
wants to merge 2 commits into from
Closed

Change tools, refactor package.json #101

wants to merge 2 commits into from

Conversation

wooorm
Copy link

@wooorm wooorm commented Aug 27, 2022

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This commit changes most tools to use the ones used in other unified packages.
Particularly, it uses prettier with xo and slightly different styles. It also uses npm.
This commit focusses on package.json. Future commits/PRs will introduce changes to other files.

@wooorm wooorm requested a review from JounQin August 27, 2022 15:20
package.json Show resolved Hide resolved
@@ -1 +0,0 @@
module.exports = require('@1stg/simple-git-hooks')
Copy link
Member

Choose a reason for hiding this comment

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

No hooks anymore?

Copy link
Author

@wooorm wooorm Aug 28, 2022

Choose a reason for hiding this comment

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

Correct. I don’t believe they are needed. Other projects maintained by unified don’t use them either.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so, it prevents unexpected codes formatting and incorrect commit message for example.

Copy link
Author

Choose a reason for hiding this comment

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

  • npm test does that too
  • hooks can be overwritten
  • hooks don’t work on PRs?
  • people will make mistakes, whether hooks are enabled or not

I can imagine they are useful when building applications or large projects.
This is a tiny package that should not require a lot of maintainance.

Copy link
Member

@JounQin JounQin Aug 28, 2022

Choose a reason for hiding this comment

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

npm test does that too

It still needs to be called manually?

hooks can be overwritten
hooks don’t work on PRs?

We install hooks on prepare hook, so if the user install dependencies, it will never be overwritten and will work on PRs.

people will make mistakes, whether hooks are enabled or not

That's why we have CI workflow.

This is a tiny package that should not require a lot of maintainance.

I'd like to be consistent for any scale of projects.

Copy link
Author

Choose a reason for hiding this comment

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

All unified projects are already consistent. I don’t believe hooks will make them more consistent.

If you feel strongly about this, I am open to having it in this project.

I’d be against adding it to every unified project. I don’t see the value in it.

Copy link
Member

@JounQin JounQin Aug 28, 2022

Choose a reason for hiding this comment

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

I’d be against adding it to every unified project. I don’t see the value in it.

Thanks, I'm not asking to use them in all unified projects.

@@ -1 +0,0 @@
module.exports = require('@1stg/lint-staged/tsc')
Copy link
Member

Choose a reason for hiding this comment

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

No lint-staged anymore?

Copy link
Author

Choose a reason for hiding this comment

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

Correct. I don’t believe it is needed. Other projects maintained by unified don’t use them either.

Copy link
Member

Choose a reason for hiding this comment

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

As above.

Copy link
Author

Choose a reason for hiding this comment

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

As above.

"type-coverage": "^2.0.0",
"typescript": "^4.0.0",
"unified": "^10.0.0",
"xo": "^0.52.0"
Copy link
Member

Choose a reason for hiding this comment

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

I don't like xo, really, and it does not check .json, .md and yml, etc. files at all.

Copy link
Author

Choose a reason for hiding this comment

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

It enforces a strict, useful, code-style. It prevents:
a) bugs,
b) having to discuss code style

Copy link
Member

Choose a reason for hiding this comment

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

My current @1stg/eslint-config already does this, and check .json, .md, .yml, etc. at the same time.

Copy link
Author

Choose a reason for hiding this comment

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

Your personal code style is not consistent with the rest of the projects maintained by us.

Copy link
Member

Choose a reason for hiding this comment

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

Your personal code style is not consistent with the rest of the projects maintained by us.

xo is chosen by you personally as I mentioned at #99

And also

But I'm also totally fine with a new eslint-config-unifiedjs.

ESLint/stylelint/prettier are the standard way and very flexible while xo is definitely not.

Copy link
Author

Choose a reason for hiding this comment

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

ESLint/stylelint/prettier are the standard way and very flexible while xo is definitely not.

ESLint is used in xo. xo provides more things than ESLint does.
Most notably, it provides not having to discuss this and getting on with fixing bugs.
The goal of xo is not to be as flexible as eslint.
xo is more standard than your personal code style.

Murderlon addressed this in #99 (comment).

But I'm also totally fine with a new eslint-config-unifiedjs.

Feel free to discuss this somewhere where everyone can participate about how we should lint 300 projects.
I am personally probably against it. xo already has good rules. xo takes away having to discuss these things.

xo is chosen by you personally as I mentioned at #99

Which sentence are you talking about?

It isn’t about personal preference of code style. That’s the whole point. It is about consistency and not having to discuss it

Copy link
Member

@JounQin JounQin Aug 28, 2022

Choose a reason for hiding this comment

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

The problem is xo doesn't handle a lot of other files like .json, .yml, etc. by https://github.com/ota-meshi/eslint-plugin-json-schema-validator for example. I'd like to have eslint-config-unifiedjs including eslint-config-xo inside with addition overrides for other files.

xo is doing well for .js/.ts, but we have other source files. So it does not provide more things than ESLint + prettier. All its values to me maybe eslint-config-xo.

Again, xo is not the standard.

@@ -0,0 +1,2 @@
*.json
Copy link
Member

@JounQin JounQin Aug 27, 2022

Choose a reason for hiding this comment

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

I don't get it.

Copy link
Author

Choose a reason for hiding this comment

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

What do you not get?

Copy link
Member

Choose a reason for hiding this comment

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

Why they're ignored by prettier.

Copy link
Author

@wooorm wooorm Aug 28, 2022

Choose a reason for hiding this comment

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

I am open to changing it.

.remarkrc Show resolved Hide resolved
package.json Show resolved Hide resolved
index.js Show resolved Hide resolved
@JounQin
Copy link
Member

JounQin commented Aug 27, 2022

According to #99 (comment), this PR is unnecessary except changing package manager, but npm is just junk in my case.

I refuse if you want to change all tools.

Because in the meantime, I could be the only maintainer of this repository.

@JounQin
Copy link
Member

JounQin commented Aug 28, 2022

Besides, I don't know how do you write the changelog file. (A CHANGELOG.md file is much easier to read than GitHub releases IMO.)

@wooorm
Copy link
Author

wooorm commented Aug 28, 2022

According to #99 (comment)

That list is:
a) all points @Murderlon finds important.
b) a separation of things he thinks all projects must do, and things he feels projects don’t have to do.

but npm is just junk in my case.

I don’t believe npm is junk.
Dropping the many custom tools that were used here, and using npm, dramatically simplified the project and made it much faster to install.

I refuse if you want to change all tools.

I strongly hope that you reconsider, and that you embrace the consistency of other unified projects.
Embracing the same style means that other people can more easily maintain this project.

Putting this project inside one of our orgs means other people will maintain it too.

I could be the only maintainer of this repository.

I do not understand what you mean.
If you mean to say that you could maintain your own project somewhere else: that is correct.
This PR is specifically meant so that other people can maintain this project.
It is expected for projects inside our organizations to be maintained by the rest of the team.

Besides, I do know how do you write the changelog file.

With Git. And I write changelog entries.

A CHANGELOG.md file is much easier to read than GitHub releases IMO.

GitHub releases has many benefits:
a) It is easier for different tools to parse, as they can use the GitHub APIs to access information.
b) Through the GitHub interface, people can watch specifically for releases.
c) GitHub shows releases to its users in their feeds, which means more people can see what is goind on with the project

@JounQin
Copy link
Member

JounQin commented Aug 28, 2022

That list is:

I agreed to align with #99 (comment)

I don’t believe npm is junk.
Dropping the many custom tools that were used here, and using npm, dramatically simplified the project and made it much faster to install.

I tried npm at #100, but I couldn't understand and make it work: https://github.com/remarkjs/remark-preset-prettier/runs/8034206256?check_suite_focus=true#step:5:32, the error message is meaningless to me. 🤣

I strongly hope that you reconsider, and that you embrace the consistency of other unified projects.
Embracing the same style means that other people can more easily maintain this project.

Putting this project inside one of our orgs means other people will maintain it too.
I do not understand what you mean.

Like eslint-mdx, I'm the only active developer, I hope other contributors can join me, of course, but before that, I'd like to keep it my way.

My point is, when no other contributors are interested, then I'd like to keep it as-is. If more contributors are coming, I'll adapt unified's common style.

GitHub releases has many benefits:

But a single CHANGELOG.md has other benefits at the same time, so in my projects, I always have them both.

@remcohaszing
Copy link

My personal preference tends to be somewhere between what @wooorm’s and @JounQin’s preferences. I agree with @wooorm though that consistency across unified projects is a good idea.

I can also see why @JounQin doesn’t really like all his preferred setup being removed. I’m thinking perhaps this project is best maintained outside of the remarkjs organization. It’s ok for people to maintain unifiedjs related projects outside of the official organizations. On the other hand I also don’t mind of a project inside a unifiedjs organization follows the rules of their sole maintainer.

@wooorm
Copy link
Author

wooorm commented Aug 28, 2022

the error message is meaningless to me. 🤣

I saw that!
I also had problems adding npm to this project at first. You know why?

  • Because of engines! 😅
  • Because of incorrect peer dependencies
  • Because of so many tools being used! (there were ±900 packages in node_modules before this commit, now there are 334)

That is why I prefer cutting down on tools. Not using a lot of (dev)dependencies.
Of course, some tools are very useful: prettier/remark to format things. xo/remark-preset-wooorm to lint things. A test and coverage framework. TypeScript.
But generally, things like hooks, things like lintstaged. Linters for every file being used here... I find that it makes it a lot harder to maintain projects.

I'm the only active developer

I hope to change that. I want to help maintain this project.
I also hope that, now you are a maintainer in several of our organizations, that you will also help maintain the other packages.

but before that, I'd like to keep it my way.

It is also the other way around: make this project consistent with the rest and maintain everything together.

My point is, when no other contributors are interested, then I'd like to keep it as-is. If more contributors are coming, I'll adapt unified's common style.

I believe this to be the reason to move projects into the organization: to maintain things together.
If someone prefers to do things their way, that’s fine! They can do that in their own project.

Like eslint-mdx

That project is a different because:

  • It is a project in @mdx-js. At the time, there were many different people and different projects in that org. It was kinda “its own separate organization”, different from the rest of unified. @mdx-js is more consistent with the rest of unified now.
  • It was from before you were a unified/mdx maintainer: you worked on that one project. Now you are a maintainer on many unified projects!

@wooorm
Copy link
Author

wooorm commented Aug 28, 2022

On the other hand I also don’t mind of a project inside a unifiedjs organization follows the rules of their sole maintainer.

I disagree on this point as there should not be sole maintainers.

@JounQin
Copy link
Member

JounQin commented Aug 28, 2022

I also had problems adding npm to this project at first. You know why?

But yarn@v1 doesn't have these issues, that's why I said npm is junk to me.

Because of engines! 😅

Not sure why engines affect this.

Because of incorrect peer dependencies

I also tried legacy-peer-deps, but it doesn't help.

Because of so many tools being used! (there were ±900 packages in node_modules before this commit, now there are 334)

xo has a lot of ESLint plugins installed, and you said:

It enforces a strict, useful, code-style. It prevents:
a) bugs,
b) having to discuss code style

I have these tools for the same purpose. And they work very well in my other projects with yarn or pnpm.

But generally, things like hooks, things like lintstaged. Linters for every file being used here... I find that it makes it a lot harder to maintain projects.

Sorry, but I disagree with this.

I hope to change that. I want to help maintain this project.

That's great, then I'm open to migrate to npm, but for hooks, I still want to keep it as-is because I often forget to run npm test before committing.

I also hope that, now you are a maintainer in several of our organizations, that you will also help maintain the other packages.

Sure, I'm trying. ❤️

It is also the other way around: make this project consistent with the rest and maintain everything together.
I believe this to be the reason to move projects into the organization: to maintain things together.
If someone prefers to do things their way, that’s fine! They can do that in their own project.

I'm trying to maintain things together, for sure. Otherwise, I won't move this project to this org.

And that's what I mean if I refuse to change all tools then I'd like to move this project to @un-ts again. I of course want to find a balance to maintain everything together, but not in a way I change everything.

I disagree on this point as there should not be sole maintainers.

It's always great to have collaborators together!


The following has not been resolved:

GitHub releases has many benefits:

But a single CHANGELOG.md has other benefits at the same time, so in my projects, I always have them both.

@wooorm
Copy link
Author

wooorm commented Aug 28, 2022

But yarn@v1 doesn't have these issues, that's why I said npm is junk to me.

I do not believe npm to be junk.
I believe the problem to be using many custom tools rather than a couple well-tested, often used, and stable tools.

xo has a lot of ESLint plugins installed, and you said:

This number is with xo. 300 folders in node_modules.
Before, there were 900 folders in node_modules. That was after yarn-deduplicate.

Having dependencies is fine. And having a lot of small dependencies is okay. But this was a lot. And slow.

I have these tools for the same purpose. And they work very well in my other projects with yarn or pnpm.

You can also use pnpm and yarn to install dependencies in this project after this PR.
This PR makes sure everything works for every package manager.
Particularly, with the one all contributors have: npm.

I still want to keep it as-is because I often forget to run npm test before committing.

See #101 (comment).
Personally, I recommend learning to run npm test (or using PRs) before committing 😉

I refuse to change all tools then I'd like to move this project to https://github.com/un-ts?type=source again

I don’t understand.
Are you interested in me working on this PR?
Or do you want to maintain this project outside of unified yourself?

But a single CHANGELOG.md has other benefits at the same time, so in my projects, I always have them both.

I personally don’t see the value of maintaining both. And I prefer releases over a file.
If you feel strongly that information should be duplicated, I am open to doing that.

@JounQin
Copy link
Member

JounQin commented Aug 28, 2022

I do not believe npm to be junk.
I believe the problem to be using many custom tools rather than a couple well-tested, often used, and stable tools.

The problem is that I don't want to install all peer dependencies manually so I have all peer dependencies as dpendencies in my @1stg/common-config and npm's auto installation could result bad result for example eslint-config-standard has not been updated from eslint@v7, I believe it could be the root cause of integration with npm failing.

xo is not friendly for TypeScript. xojs/eslint-config-xo-typescript#59

ESLint/prettier themselves are well-tested, eslint-config-xo has only two test cases.

https://github.com/xojs/eslint-config-xo/blob/main/test/test.js

Having dependencies is fine. And having a lot of small dependencies is okay. But this was a lot. And slow.

Slow: It can be slow, but that’s because it includes a lot of useful rules

mdx-js/mdx#2065 (comment)

And I didn't see it's slower than xo.

You can also use pnpm and yarn to install dependencies in this project after this PR.
This PR makes sure everything works for every package manager.
Particularly, with the one all contributors have: npm.

How are you marking sure? I don't see any test cases for other package managers.

Personally, I recommend learning to run npm test (or using PRs) before committing 😉

I'm getting older and older, can't remember such tiny things. And we have tools to prevent, why not?

I don’t understand.
Are you interested in me working on this PR?
Or do you want to maintain this project outside of unified yourself?

I am, but don't see the point to change all tools that affect me personally a lot like @remcohaszing's comments above. 😅

#101 (comment)

I personally don’t see the value of maintaining both. And I prefer releases over a file.
If you feel strongly that information should be duplicated, I am open to doing that.

Thanks, I'd like to have CHANGELOG.md at the same time. Releases can have paginations, while a CHANGELOG.md can be viewed easier.

@wooorm
Copy link
Author

wooorm commented Aug 29, 2022

peer dependencies

Peer dependencies have been causing problems for years now.
E.g.:

npm's auto installation could result bad result for example eslint-config-standard has not been updated from eslint@v7, I believe it could be the root cause of integration with npm failing.

I believe this to be a problem with using unmaintained or out of date dependencies, instead of a problem with npm.

xo is not friendly for TypeScript. xojs/eslint-config-xo-typescript#59

I see xo in many repos that use TypeScript.
Furthermore, in unified, we use TypeScript through JSDoc, so if there was an issue, it is not an issue for us.
And finally, you link to a problem caused by eslint/eslint#3458. A 7 year old issue with more than 500 👍s. If this says anything, it is that ESLint isn’t very good, not that xo is not very good.

ESLint/prettier themselves are well-tested, eslint-config-xo has only two test cases.

This is a false comparison.
Particularly in a project that has no tests at all.
Your ESLint presets have very few tests, too.
xo has tons of tests. Each rule has tons of tests.

Your style presets are used by just you: https://github.com/1stG/configs/network/dependents.
xo is used in thousands of projects: https://github.com/xojs/xo/network/dependents?package_id=UGFja2FnZS0xODM5Mjg4NA%3D%3D, https://github.com/xojs/eslint-config-xo/network/dependents?package_id=UGFja2FnZS0xNDg1NjExNA%3D%3D.

And I didn't see it's slower than xo.

Ignoring package locks, install time of this repo was very slow. It is now fast.

How are you marking sure? I don't see any test cases for other package managers.

Please try it out! It works because:
a) there are a lot less dependencies, meaning there are a lot less conflicts
b) no engines, no peers

I'm getting older and older, can't remember such tiny things. And we have tools to prevent, why not?

There are tools for everything. This repo does not use every tool.
Tools come at a cost.
You pick and choose what is most useful.
Testing code before committing is something I believe that should be as normal as wearing a coat when it’s cold outside.

like @\remcohaszing's comments above

And I agree with Remco:

  • if you want to maintain things on your own, using your personal style and tool preferences, maintain it outside our orgs. That’s fine!
  • if you want to the unified collective to maintain this, use the styles and tools consistently used in every project

@JounQin
Copy link
Member

JounQin commented Aug 29, 2022

I don't want to argument these meaningless things anymore. 😅

And I agree with Remco:

OK, let's move this repository back to @un-ts, I'm sorry.

@wooorm
Copy link
Author

wooorm commented Aug 29, 2022

I am not a maintainer of un-ts so I can’t transfer repos there.
I transferred to myself. Then to you. You can do the rest.

@wooorm wooorm closed this Aug 29, 2022
@wooorm wooorm deleted the package branch August 29, 2022 07:54
@JounQin
Copy link
Member

JounQin commented Aug 29, 2022

Thanks! @wooorm

That's my bad for not raising an issue for discussion first.

@wooorm
Copy link
Author

wooorm commented Aug 29, 2022

no problem! :)

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 this pull request may close these issues.

None yet

3 participants