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

Create RFC for async Compile plugin #7

Closed
wants to merge 2 commits into from

Conversation

rkesters
Copy link

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

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

We’ve already discussed this here:

You have a particular problem: making PDFs from markdown.
That task can be done sync inside unified (unifiedjs/unified-engine#62 (comment)) or in any way you want outside of unified


If we implement this proposal, how will existing unified developers adopt it?

* It will be transparent to all unified developers, expect does developing Compile plugins
Copy link
Member

Choose a reason for hiding this comment

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

It would break anyone using .stringify or Compile


* It will be transparent to all unified developers, expect does developing Compile plugins

Is this a breaking change? `No `
Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Why should we *not* do this?
Please consider:

No clear drawbacks,
Copy link
Member

Choose a reason for hiding this comment

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

It would break anyone using .stringify or Compile

to return a promise. Additionally, this will ensure that plugins loaded via cli (unified-args)
function the same as plugins loaded via `.use()`.
What use cases does it support?
Cases when Compile is naturally async.
Copy link
Member

Choose a reason for hiding this comment

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

I do not believe there are cases where Compile is naturally async as you say.
The role of Compile is to turn a syntax tree (a complete document in AST form) into a string. There is no reason for that task to ever be async.

Copy link
Author

Choose a reason for hiding this comment

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

You are precluding the use of stream.Writable , which is a common why to build up a string. Also prevents the use of a micro-service, or as any lib that might need a resource from the internet.

You stated somewhere else that pdfMake has a sync functionality, if it does I can not find it; give that pdfMake is based on pdfKit which is based on node streams.

Copy link
Member

Choose a reason for hiding this comment

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

You are precluding the use of stream.Writable

Yes. Because streaming and ASTs don‘t make sense. PostCSS doesn’t stream. Babel doesn’t stream.

Streaming makes sense because some chunk of data (e.g., binary, text) on disk is (or can be) soooo big that it would become slow to handle in one go and better to handle it in chunks.
But, async work is always slower. fs.readFileSync() is always faster than fs.readFile. But the second has other benefits.

unified has access to the whole tree (the AST) already in memory. The size of the AST in memory is much bigger than the source of the document. Because we have everything already, serializing that tree to plain-text never has to be sync.

which is a common why to build up a string.

Streams and ASTs are not commonly combined.

Also prevents the use of a micro-service, or as any lib that might need a resource from the internet.

No. Not at all. Streaming exists outside of unified. Note that you’re talking about parsing here, where this issue/RFC is about serializing.

@wooorm wooorm closed this Mar 15, 2022
@wooorm wooorm added 🤷 no/invalid This cannot be acted upon 👎 phase/no Post cannot or will not be acted on labels Mar 15, 2022
@rkesters
Copy link
Author

Why are these being closed without community discussion?

Also this change is not a breaking change, becuase no calls stringify directly, it is always called by trough.

@wooorm
Copy link
Member

wooorm commented Mar 15, 2022

becuase no calls stringify directly

Many people use the unified API: https://github.com/unifiedjs/unified#processorstringifynode-file.

Why are these being closed without community discussion?

They’re closed because it isn’t a good idea and you ignore the alternatives and offerings of help.

“community discussion” !== “one person on the internet wants something”.

@rkesters
Copy link
Author

becuase no calls stringify directly

Many people use the unified API: https://github.com/unifiedjs/unified#processorstringifynode-file.

Why are these being closed without community discussion?

They’re closed because it isn’t a good idea and you ignore the alternatives and offerings of help.

“community discussion” !== “one person on the internet wants something”.

"community discussion" !== "one maintainer closing tickets"

"community discussion" === "having a steering committee that reviews and votes on an RFC after multiple members of the community have review and provided comments"

But I understand that you have your opinion and that is why gitbub has forking.

@wooorm
Copy link
Member

wooorm commented Mar 19, 2022

"community discussion" === "having a steering committee that reviews and votes on an RFC after multiple members of the community have review and provided comments"

RFCs go through that process.
This is not an RFC.
This is an issue. The issue was invalid. You didn’t agree.

This project is run mostly by volunteers/community. Apparently the community does not care for your issue.
If you want to help maintain unified and start reviewing issues and RFCs, you can find everything you need here: https://github.com/unifiedjs/collective.


You stated somewhere else that pdfMake has a sync functionality, if it does I can not find it; give that pdfMake is based on pdfKit which is based on node streams.

Can you please ask such questions before picking a whole fight? Like. I’ve spent so much time arguing with you. While I’ve already answered it...

https://pdfmake.github.io/docs/0.1/getting-started/client-side/methods/

But I also said that there are many ways to generate a PDF. Not just PDFMake.
I personally have made beautiful documents by turn markdown (remark) into HTML (rehype) and then feeding that into PrinceXML / DocRaptor.

But the import part is: streaming does not fit inside unified. It exists outside of unified.


I’m locking this thread because it’s off-topic. Feel free to ask questions about how to generate PDFs in Discussions.

@unifiedjs unifiedjs locked as off-topic and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🤷 no/invalid This cannot be acted upon 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

2 participants