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

feat: add compatible sync API support #157

Closed
wants to merge 4 commits into from
Closed

feat: add compatible sync API support #157

wants to merge 4 commits into from

Conversation

JounQin
Copy link
Contributor

@JounQin JounQin commented Apr 26, 2021

close #153, close #155

cc @YusukeHirao

It should be the minimum changes.

I'm just doing coping and pasting for now to give you a review first.


loop();

return promise;
Copy link
Member

Choose a reason for hiding this comment

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

@JounQin

This return promise means own function is an async function.
I guess it didn't need change.
Use await.

Copy link
Contributor Author

@JounQin JounQin Apr 27, 2021

Choose a reason for hiding this comment

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

@YusukeHirao

You're right, changed the type declaration in f1f7efc.

for async API, the execution flow is just the same as before.

But await or promise.then in original loop will be executed in the next tick, so in sync mode, the previous logic is incorrect.

Promise.resolve().then(() => console.log(1));
console.log(2);

// 2
// 1
const testAsync = async () => {
  for (const node of [1, 2, 3]) {
    console.log('async', await node)
  }
}

testAsync()
console.log('sync:', 1)

// sync: 1
// async 1
// async 2
// async 3

Copy link
Contributor Author

@JounQin JounQin Apr 27, 2021

Choose a reason for hiding this comment

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

const sleep = (timeout = 0) =>
  new Promise(resolve => setTimeout(resolve, timeout))

const testAsync = walker => {
  let _resolve
  let _reject

  const promise = new Promise((resolve, reject) => {
    _resolve = resolve
    _reject = reject
  })

  const nodeList = [1, 2, 3]

  const loop = (index = 0) => {
    if (index >= nodeList.length) {
      return _resolve()
    }
    const node = nodeList[index]
    const result = walker(node)
    if (result instanceof Promise) {
      console.log('async walker:', result)
      result.then(() => loop(index + 1)).catch(_reject)
    } else {
      console.log('sync waker:', result)
      loop(index + 1)
    }
  }

  loop()

  return promise
}

testAsync(node => {
  console.log('node:', node)
  return node
})

testAsync(async node => {
  await sleep(Math.random() * 1000)
  console.log('node:', node)
  return node
})

console.log('sync:', 0)

The above code shows the difference more obviously, you can test it in node fast.

node: 1
sync waker: 1
node: 2
sync waker: 2
node: 3
sync waker: 3
async walker: Promise { <pending> }
sync: 0
node: 1
async walker: Promise { <pending> }
node: 2
async walker: Promise { <pending> }
node: 3

Copy link
Member

Choose a reason for hiding this comment

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

@JounQin

Same:

const testAsync = async walker => {
	const nodeList = [1, 2, 3];
	for (const item of nodeList) {
		await walker(item);
	}
	return;
};

Copy link
Contributor Author

@JounQin JounQin Apr 27, 2021

Choose a reason for hiding this comment

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

@YusukeHirao

const sleep = (timeout = 0) =>
  new Promise(resolve => setTimeout(resolve, timeout))

const testAsync = async walker => {
  const nodeList = [1, 2, 3]
  for (const item of nodeList) {
    await walker(item)
  }
}

testAsync(node => {
  console.log('sync node:', node)
  return node
})

testAsync(async node => {
  await sleep(Math.random() * 1000)
  console.log('async node:', node)
  return node
})

console.log('sync:', 0)
sync node: 1
sync: 0
sync node: 2
sync node: 3
async node: 1
async node: 2
async node: 3

It's not correct, sync: 0 should be logged after sync node: 3

Copy link
Member

Choose a reason for hiding this comment

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

@JounQin

I must change sync APIs every if I want to change any inner logic in async APIs. There are 800 lines over.
Umm..., I DO NOT WANT THAT.

Your idea goes to force that.

Copy link
Contributor Author

@JounQin JounQin Apr 27, 2021

Choose a reason for hiding this comment

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

@YusukeHirao

Yeah, that's right, async and sync are very different.

I've mentioned that I'm just doing coping and pasting for now, maybe the same logic between some of them can be refactored, maybe not. Most of the cope-paste codes are for file-resolver. Ideally they should not be changed often.

But of course, if you don't like my current idea, this PR can be closed. I've found a way to call async API synchronously on Node.js, it will just lose some performance.

A better approach would be awesome too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option is reverting file resolver changes but keep ability for sync rule, and then I'll create an package maybe markuplint-sync to extend the ability for linting synchronously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YusukeHirao

How do you think? I'd like to work on this approach tonight.

Copy link
Member

Choose a reason for hiding this comment

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

@JounQin

Oh, I see.
This package is under the MIT license.
You create the package is freedom.
I don't have the right to stop you.

If you created it, I want to see it.

@YusukeHirao
Copy link
Member

@JounQin

OK, I don't accept this code.
But I want to accept code if that low cost and simple.
Please you another PR if you find out the better approach.

By the way, would you PR if you found out some typo?

And I think I'm removing the unneeded async keywords;)

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.

[feat] Can you please provide sync API with async together?
2 participants