feat: support sync API #155

JounQin · 2021-04-24T04:40:20Z

close #153

All async APIs should have sync version now, I'll add all test cases for sync APIs too by extracting an util testAsyncAndSyncVerify for rules, and it's just working.

Some refactor to extract same logic between async and sync is needed too.

JounQin · 2021-04-25T02:11:43Z

cc @YusukeHirao

JounQin · 2021-04-25T02:20:16Z

packages/@markuplint/file-resolver/src/ml-file.ts

@@ -32,17 +31,23 @@ export class MLFile {
 		return this.#filePath;
 	}

-	async isExist() {


There is no usage of this method in the whole code base, so I remove it here, if you think it's unexpected, I'll revert it back and add a sync method too here.

YusukeHirao · 2021-04-25T02:20:31Z

@JounQin Thank you for PR.

But, I can not evaluate this. It's difficult for me because it's too huge.

Firstly, why you need sync APIs? I'm satisfied with only async APIs, And I've never feel need that.

It increased codes similar to async APIs. In other words, it increased the cost for maintenance to code I must.

I need always your help from now on if I approve.

JounQin · 2021-04-25T02:22:48Z

packages/@markuplint/ml-core/src/ml-dom/helper/walkers.ts

 import { RuleConfigValue } from '@markuplint/ml-config';

-export type Walker<T extends RuleConfigValue, O = null, N = AnonymousNode<T, O>> = (node: N) => Promise<void>;


I moved them into types files to be constantly and exports them all to reuse them in other packages.

packages/@markuplint/ml-core/src/ml-rule/types.ts

JounQin · 2021-04-25T02:41:54Z

@YusukeHirao

Thanks for your reply first.

Firstly, why you need sync APIs?

As I mentioned at #153, async APIs are not compatible with some tools like ESLint currently, I want to develop an ESLint plugin eslint-plugin-markup for linting markup language syntaxes(not ES syntaxes).

It increased codes similar to async APIs. In other words, it increased the cost for maintenance to code I must.

I need always your help from now on if I approve.

I don't think that's true, I've extracted all sharable codes between async and sync APIs, they are almostly for rules. The differences between async and sync APIs are very small except await and Sync suffix. Actually, as I noticed, all the walkers are implemented synchronized althogh they are marked as async.

You don't need any help from me because they are just the same/similar. Of course, I'd glad to help if you ask.

Providing a sync API is great for other plugin authors too in my mind.

I hope markuplint will union all html related linters like htmlhnit, htmllint or linthtml, etc.

The other html linters as I mentioned all provide sync APIs.

It would be much simpler to migrate if we provied the sync APIs.

JounQin · 2021-04-25T02:48:47Z

Besides, I had a similar discussion with textlint's author at textlint/textlint#756.

And I'll raise a PR there too.

JounQin · 2021-04-25T02:51:44Z

Oh, I've forgotten to mention that the diff is huge because I added all test cases for sync APIs, the codes for sync is not huge.

JounQin · 2021-04-25T04:53:28Z

When I implement textlint/textlint#757, I realized that the rule API of textlint actually support async and sync at the same time

And most of the rules can be sync actually, so if we change all rules just be synchronized, the PR would be much cleaner.

All to do is to change the Walker signature as (...) : <T> | Promise<T> and change current rules to be sync, and no SyncWalker is needed any more.

YusukeHirao · 2021-04-25T06:02:08Z

@JounQin

Ok, could you debate it step by step?

Firstly, I think don't need sync APIs. Because an async API can accept a sync API. In short words, An async API can include a sync API.
In fact, createRule function accepts an async function or a sync function as a param.

If I accept the sync APIs, a user can not create a custom rule on async.
If run verifySync does it throw an error, isn't it? Or does it ignore async rules, isn't it? I can not approve.
Can create custom rules on async, it is a value.
The rules will be running equality.
(I think textlint has the same policy.)

JounQin · 2021-04-25T06:11:59Z

@YusukeHirao

Ok, could you debate it step by step?

Oh, sorry, I was just thinking fast when coding on texlint PR.

In fact, createRule function accepts an async function or a sync function as a param.

For now, it's not true, because its signature is Promise<T> in ts:

https://github.com/markuplint/markuplint/blob/master/packages/%40markuplint/ml-core/src/ml-rule/types.ts#L10-L11

Change it to T |Promise<T> would help as I mentioned

If I accept the sync APIs, a user can not create a custom rule on async.

That's not what I'm doing. I'm just trying to expose a compatible interface with sync API support.

If run verifySync does it throw an error, isn't it? Or does it ignore async rules, isn't it? I can not approve.

It should throw an error when call sync API on async only rules, just like unified and my implementation for markuplint and textlint now, so that the users can know the limitation and disable the related rules accordingly.

See also

textlint/textlint#756 (comment)
https://github.com/textlint/textlint/pull/757/files#diff-a40ec1e594b82fb2e43799bdfac8df8bc76fae39889f362c2c1df6f9e021aa36R37

Can create custom rules on async, it is a value.
The rules will be running equality.

Yes, and the users can just disable them on calling sync APIs.

I'm not going to force plugin authors to provide sync APIs, but it would be great if it is possible like all rules currently except rule-textlint for now.

I can change this PR to remove verifySync/fixSync/warkSync/warkOnSync but change verify/fix/warlk/warkOn signature and change all rules to be synchronized instead if you agree.

This would make this PR much simpler and cleaner.

YusukeHirao · 2021-04-25T06:37:46Z

@JounQin
Could you tell me eslint-plugin-markup that you develop?
I develop jsx-parser just now.

Would you help develop if your purpose is the same?

JounQin · 2021-04-25T06:42:40Z

@YusukeHirao

It seems jsx-parser aims ES syntaxes too.

eslint-plugin-markup aims to integrate markuplint for html like syntaxes but as an ESLint plugin.

Its like eslint-plugin-mdx which integrates remark-lint together to lint markdown syntaxes for .md and .mdx files.

But it would be greater that can be used for jsx! Is there anything I can offer help?

YusukeHirao · 2021-04-25T07:06:26Z

@JounQin
It means don't need your eslint-plugin-markup if there is @markuplint/jsx-parser.

Does eslint-plugin-markup search HTML in JSX or TSX, doesn't it?

@markuplint/jsx-parser extracts HTML(and JSX elements) from JSX and TSX so ignores other syntaxes and tokens. Only verifies markup syntaxes.

It is very simple. Isn't this what you want?

JounQin · 2021-04-25T07:10:48Z

Does eslint-plugin-markup search HTML in JSX or TSX, doesn't it?

No, it lints .html files. But it can be better as you suggested.

YusukeHirao · 2021-04-25T07:14:34Z

@JounQin
Why don't use markuplint?

JounQin · 2021-04-25T07:16:11Z

@JounQin
Why don't use markuplint?

So that I can use only one command eslint . for all files.

It would be much easier after eslint/rfcs#56 been implemented.

YusukeHirao · 2021-04-25T07:29:42Z

@JounQin
Oh, I see. I understood what you want.
But, I think your PR is huge even more.
Do you use NPM scripts in package.json?

"scripts": {
  "lint": "eslint --xxx xxx; markuplint --xxx xxx;"
}

JounQin · 2021-04-25T07:39:00Z

But, I think your PR is huge even more.

Yes, I'm simplifying based on #155 (comment)

It will be much cleaner.

Do you use NPM scripts in package.json

Yes, but I still want a union linter, so that I can use shared eslint config for all projects quickly.

Imagine that we've had a lot of linters for different files, x for *.a, y for *.b, z for *.c, etc.

We need to run x *.a && y *.b && z *.c...

My purpose is a single linter for all files.

JounQin · 2021-04-25T07:58:31Z

@YusukeHirao It should be much cleaner to review now. Most changes are test cases for test sync API at the same time. And a lot of changes are just changing current rules to be synchronized but keep rule-textlint because it requires to be async now.

The must-been-reviewed files are:

packages/@markuplint/ml-core/src/ml-dom/document.ts: https://github.com/markuplint/markuplint/pull/155/files#diff-e96bd6a757d03287d3bb4b932c4cd429d89ca106ad4cd226baf656f88d47d5c3

packages/@markuplint/ml-core/src/ml-core.ts: https://github.com/markuplint/markuplint/pull/155/files#diff-d172128795f532b105615430f26b8ec7303b4c0efe4f6697ab2769574f7861e1

packages/@markuplint/ml-core/src/ml-rule/ml-rule.ts: https://github.com/markuplint/markuplint/pull/155/files#diff-258266a7d4660e36f81a70bdad016431829bc10e49a8f129095791ffffd0a2f0

And the rest changes are just providing necessary *Sync functions.

JounQin · 2021-04-25T15:12:08Z

@YusukeHirao

Now that I've implemented to use markuplint async API but as sync via child_process which is inspired by sync-threads actually, this PR can be closed if you really don't want this feature.

But I still want to say that it would be great to support sync API for performance reason like my integration. (serialization/deserialization between calls and io costing for files)

YusukeHirao · 2021-04-26T03:07:24Z

@JounQin
I thought last night, and decide that I don't approve.

Your idea goes to force plugin authors to provide the rule that an async and a sync dividedly. It violates the policy even if users can ignore async rules when run. Because it is "complex". I hope users can lint without caring about async/sync.

And the first reason is you have only to don't use the one command. I feel you. I feel you. I feel you. And your refactoring is great! But you should do another approach. Why do you develop the wrapper command that includes ESLint, markuplint, and textlint? The wrapper command can include async linters and sync linters. It has a universality that can support a new unknown linter that will be born from now on.

JounQin · 2021-04-26T03:20:00Z

@YusukeHirao

Your idea goes to force plugin authors to provide the rule that an async and a sync dividedly.

It's not true in my latest implementation.

The plugin authors can choose to write the rule async or sync, it just fails to run async rules in sync mode. And then the users can just disable the related rules. And also, the users using @markuplint/cli won't be affected at all, because the cli is still always running on async mode.

Why do you develop the wrapper command that includes ESLint, markuplint, and textlint?

Because ESLint itself is just doing it as I mentioned.

eslint/rfcs#42
eslint/rfcs#56

It's just not ready for now.

YusukeHirao · 2021-04-26T03:51:31Z

@JounQin

Well if it, you should wait to complete the new feature of ESLint.

YusukeHirao · 2021-04-26T04:25:26Z

@JounQin

I accept your passion.
So I want to see the new logic.
Well, Would you clean your code?
Please change the code to below:

Don't change names (of method, function, and property). Please create another PR if you found the typo.
Don't change tests. (I guess it can do if add only your sync logic.)
Add tests for a new logic.
Change only the minimum required.

If it, I can see it.

JounQin · 2021-04-26T04:47:34Z

OK, I'll start a new PR from scratch accordingly.

YusukeHirao · 2021-04-27T03:24:35Z

Close this because there is #157.

feat: support sync API

04434ab

JounQin mentioned this pull request Apr 24, 2021

[feat] Can you please provide sync API with async together? textlint/textlint#756

Closed

test: enable all test cases of rules for sync API

2554475

JounQin marked this pull request as ready for review April 24, 2021 06:22

JounQin added 3 commits April 24, 2021 16:53

test: enable more sync API related cases

1cbaa04

docs: document about sync API

eaa75e1

refactor: extract same logic between async/sync APIs for rules

ba6ec23

JounQin commented Apr 25, 2021

View reviewed changes

packages/@markuplint/ml-core/src/ml-rule/types.ts Show resolved Hide resolved

JounQin commented Apr 25, 2021

View reviewed changes

packages/@markuplint/ml-core/src/ml-rule/types.ts Outdated Show resolved Hide resolved

JounQin mentioned this pull request Apr 25, 2021

feat: support sync API textlint/textlint#757

Closed

refactor: change walker signature

25e641c

JounQin mentioned this pull request Apr 26, 2021

feat: add compatible sync API support #157

Closed

YusukeHirao closed this Apr 27, 2021

JounQin deleted the feat/sync_api branch April 27, 2021 04:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support sync API #155

feat: support sync API #155

JounQin commented Apr 24, 2021 •

edited

JounQin commented Apr 25, 2021

JounQin Apr 25, 2021 •

edited

YusukeHirao commented Apr 25, 2021

JounQin Apr 25, 2021

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 •

edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 •

edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 •

edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 •

edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021

JounQin commented Apr 25, 2021 •

edited

YusukeHirao commented Apr 26, 2021

JounQin commented Apr 26, 2021 •

edited

YusukeHirao commented Apr 26, 2021

YusukeHirao commented Apr 26, 2021 •

edited

JounQin commented Apr 26, 2021

YusukeHirao commented Apr 27, 2021

		import { RuleConfigValue } from '@markuplint/ml-config';

		export type Walker<T extends RuleConfigValue, O = null, N = AnonymousNode<T, O>> = (node: N) => Promise<void>;

feat: support sync API #155

feat: support sync API #155

Conversation

JounQin commented Apr 24, 2021 • edited

JounQin commented Apr 25, 2021

JounQin Apr 25, 2021 • edited

Choose a reason for hiding this comment

YusukeHirao commented Apr 25, 2021

JounQin Apr 25, 2021

Choose a reason for hiding this comment

JounQin commented Apr 25, 2021 • edited

JounQin commented Apr 25, 2021

JounQin commented Apr 25, 2021 • edited

JounQin commented Apr 25, 2021 • edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 • edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 • edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 • edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 • edited

YusukeHirao commented Apr 25, 2021

JounQin commented Apr 25, 2021 • edited

JounQin commented Apr 25, 2021

JounQin commented Apr 25, 2021 • edited

YusukeHirao commented Apr 26, 2021

JounQin commented Apr 26, 2021 • edited

YusukeHirao commented Apr 26, 2021

YusukeHirao commented Apr 26, 2021 • edited

JounQin commented Apr 26, 2021

YusukeHirao commented Apr 27, 2021

JounQin commented Apr 24, 2021 •

edited

JounQin Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 25, 2021 •

edited

JounQin commented Apr 26, 2021 •

edited

YusukeHirao commented Apr 26, 2021 •

edited