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

Use Swirly to generate PNGs #5019

Closed
wants to merge 6 commits into from
Closed

Use Swirly to generate PNGs #5019

wants to merge 6 commits into from

Conversation

timdp
Copy link
Contributor

@timdp timdp commented Sep 15, 2019

While I think it's great that the official RxJS docs come with many marble diagrams, it's always kind of bothered me that the text in there is poorly antialiased. Additionally, I feel like they could be annotated here and there to make it clear what's going on.

Now, I'm about to do a talk on Rx, so I needed a quick way to generate diagrams. Some tools exist already, but I couldn't find any that would let me render higher-order observables. Additionally, I still would have had to annotate the resulting images manually for my talk.

As a result, I set out to build my own diagram generator called Swirly. It generates diagrams from a text-based declaration and exports them to SVG. That means high-res vector graphics and hopefully an extensible format for annotations. The diagrams could also be instrumented for interactivity a la rxmarbles.com, but that's hypothetical at this point. There's a live demo over at swirly.now.sh.

While Swirly isn't mature yet, I already had a stab at porting the RxJS code that generates PNGs to a format that Swirly understands. The result is this PR. As far as I can tell, the diagrams are identical to the existing ones, but I'll admit it's not the easiest thing to test. Here's an example:

combineAll

For compatibility reasons, I added some boilerplate code that renders the SVGs to PNG using Puppeteer. Additionally, I force-committed the Git-ignored images for demonstration purposes. Both steps are optional; the docs could theoretically also include crisp SVGs.

I'm not necessarily advocating replacing the current images with Swirly-generated ones, but if there's any appetite for that, I'm happy to help out further.

@benlesh
Copy link
Member

benlesh commented Oct 15, 2019

attn @jwo719

@niklas-wortmann
Copy link
Member

Wow those images are amazing. I would happy to merge, would you mind resolving the conflicts, right after it I am happy to merge it!

@timdp
Copy link
Contributor Author

timdp commented Oct 19, 2019

Wow, I wasn't expecting such a positive response. 🙂 Sure, let me resolve the conflicts.

@timdp
Copy link
Contributor Author

timdp commented Oct 19, 2019

I unfortunately ran into two issues when rebasing:

  1. With master on 1b2021f, a clean npm i gives me:

    npm ERR! code E404
    npm ERR! 404 Not Found - GET https://registry.npmjs.org/es-abstract/-/es-abstract-1.14.0.tgz
    npm ERR! 404 
    npm ERR! 404  'es-abstract@1.14.0' is not in the npm registry.
    

    I was able to resolve this by manually removing the entry for es-abstract and running npm i again.

    EDIT: Reported as package-lock.json references nonexistent es-abstract@1.14.0 #5099.

  2. However, having rebased my code on top of that, Swirly renders several diagrams as empty.

    Now, this also appears to be the case on master. For example, this is how audit renders:

    audit

    EDIT: Reported as tests2png is defeated by TestScheduler#run() #5100.

Am I doing something wrong or should I file issues for these?

@timdp
Copy link
Contributor Author

timdp commented Oct 20, 2019

So I used git bisect to compare the latest commit on master (bad) with fc3d426 (good), which is where this PR originally forked. At every bisection step, I ran:

git clean -fdX && \
  npm i es-abstract@1.14.2 && \
  npm i && \
  npm run tests2png -- --grep '.*audit.*'

The es-abstract part is a workaround for #5099. I'm 99% sure it doesn't affect this bug.

I focused on audit and auditTime, which currently render as empty on master. Other images seem to be affected though.

  • For audit, the behavior started in c41d00b.

  • For auditTime, the behavior started in bc91ba0.

Both these commits introduce TestScheduler#run() into the tests, so it looks like that change is incompatible with the code that generates the PNGs (and therefore also with my PR).

@timdp
Copy link
Contributor Author

timdp commented Oct 20, 2019

As can be seen above, I created follow-up tickets #5099 and #5100 for the issues I encountered.

In the meantime, I've left this PR in its original state. However, the rebased (and therefore broken) version is also available, as my swirly-rebased branch.

@timdp timdp changed the title Use Swirly + Puppeteer to generate PNGs Use Swirly to generate PNGs Oct 21, 2019
@niklas-wortmann
Copy link
Member

@timdp Coud you maybe try rebasing this, afterwards I will try to pick it up and support you as much as possible

@timdp
Copy link
Contributor Author

timdp commented May 8, 2020

I've rebased my swirly-rebased branch against 02f458d on master. The issues from #5100 remain though, so I didn't push to this PR yet.

@timdp
Copy link
Contributor Author

timdp commented May 27, 2020

Closing this in favor of #5422.

@timdp timdp closed this May 27, 2020
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