rxjs
rxjs copied to clipboard
Marble diagrams for display in inline JsDocs are missing from npm package
Bug Report
Current Behavior The inline JsDoc/TypeDoc often make reference to images using relative paths, but those images aren't part of the npm bundle (for as far as I can tell), and thus the Inline docs, when rendered in my IDE, are missing those images.
For example, see the docs for combineLatest:
/**
* Combines multiple Observables to create an Observable whose values are
* calculated from the latest values of each of its input Observables.
*
* <span class="informal">Whenever any input Observable emits a value, it
* computes a formula using the latest values from all the inputs, then emits
* the output of that formula.</span>
*
* 
Expected behavior I'd expect the images to be displayed.
Reproduction
For example, in IntelliJ, open a rxjs ts file (e.g. node_modules/rxjs/src/internal/observable/combineLatest.ts), click into a method definition (with extensive comment that makes use of an image, e.g. around line 226 there), and choose View > Quick Documentation.
Please provide repro code does not involves framework, or other setups to address this bug is specific to rxjs core n/a
Environment n/a
Possible Solution
Not sure how this can be achieved with npm, but the images could potentially be shipped in a way that they don't get redistributed to end users and are available in IDEs only. Alternatively, URLs to the rxjs webserver could potentially be used (instead of relative paths).
Additional context/Screenshots

Hmm... These should probably link to web urls. Because our npm package is already enormous.
I don't think we can easily achieve this with our current docs generation setup. Basically, what we do right now is that we extract JSDoc comments from exported items, parse it and in case of images, we replace such lines with the real images.
So, in our code, if we'd replace e.g.

with

we'd have to update our embedMarbleDiagrams.js processor.
But, we'd have to let everybody know how to embed new or replace existing images in potential new PRs because having absolute URLs would have to be stripped in the embedMarbleDiagrams.js and then only the last URL path (in this case: combineLatest.png) would have to be used so we can embed the image properly. It's not impossible, but given the time we have on this project, I don't think we could achieve it in a timely manner.
Also, our marble diagrams need to go through massive face-lifting (we also have a new way of writing marble diagrams by using SVG images instead of PNG images that we used before).
@niklas-wortmann, what do you think about this? Would using absolute paths (like this one: ) create issues to maintainers that would like to update/create new images for existing operators - they'd have to know that they'd have to use absolute URL, but only the file name would be used to inject the image properly.
@anno1985, I'm open for suggestions or possibly a PR that would address this issue properly (with both embedMarbleDiagrams.js fix and a guide how to include existing images to the docs with the absolute URL).
I did experiment with this a while ago. While it technically works as you described above, the problem is, those images are comparatively big right now (e.g. switchMap's marble diagram is 1280 × 740 px). What happened now is that I rendered the full size image in the little popup, so it was blowing up the popup. While I'd like to see the marble diagrams myself, I did not like the additional noise generated
Yes, I found this issue as well:

It could've been solved with migrating to SVG images which are usually much smaller:

(IDE that I use has a bug and it only displays black images, but this is how it would look like ☝️).
But that's a lot of work that we need to invest. And everyone would have to know how to write a proper syntax. Also, we'd have to keep all old images in our repository for anyone with the old versions of the library to be able to access those images from our site.
It's black because the IDE doesn't support all sorts of colors as rgba and hsl. If replaced with HEX (like #00aaff) it should work.
Here I have developments with diagrams that:
- [x] Drawing text with
<path />. No need to worry about the presence of the font. - [x] Dynamic calculation of stream title width.
- [x] Dynamic calculation of operator width.
<foreignObject />is not used. - [x] Lack of sub pixel coordinates. Clearer display.
- [x] Rendering without browser APIs. Can be easily run in Node.js.
- [x]
complete&errornow have the same radius asnext. Butcomplete&errorbecome larger ifnextoverlaps them. Just like here. - [x] Built-in dark theme!
Is this topic interesting?