solid-plotly.js a new wrapper for Plotly.js
I could use some more experienced eyes to check the SolidJS implementation details. Eventually I'd like to get this into the official plotly.js distributions.
https://github.com/ralphsmith80/solid-plotly.js/tree/main
I've posted on there support forum about that as well.
https://community.plotly.com/t/solidjs-version-of-plotly-could-use-some-eyeballs/87761
I want to use Plotly with SolidJS. The closest match would be React, but they’re fundamentally different. I used the React project to build a similar wrapper using SolidJS. ralphsmith80/solid-plotly.js: A plotly.js SolidJS component from Plotly (github.com) It’s a monorepo using pnpm. The solid-plotly.js packages is under packages and the demo app is under apps. I’d like to see about getting this included into the available plotly packages. I could use some eyes on the project and there’s still some work around testing and documentation that needs to be done. In particular I’m struggling to get the same tests working that are in the react repository. The differences revolve around the expectPlotlyAPICall test function and I’m sure it’s related to the fact that I wrote the solidjs version using TS which I’m not very good at yet. For docs the README pages need to be updated.
GitHub
GitHub - ralphsmith80/solid-plotly.js: A plotly.js SolidJS componen...
A plotly.js SolidJS component from Plotly. Contribute to ralphsmith80/solid-plotly.js development by creating an account on GitHub.
Plotly Community Forum
SolidJS version of Plotly - Could use some eyeballs
I want to use Plotly with SolidJS. The closest match would be React, but they’re fundamentally different. I used the React project to build a similar wrapper using SolidJS. ralphsmith80/solid-plotly.js: A plotly.js SolidJS component from Plotly (github.com) It’s a monorepo using pnpm. The solid-plotly.js packages is under packages and the demo...
53 Replies
i had a quick look at it, i don't know plotly and haven't run your code so take my words with a grain of salt.
code looks pretty good, don't see any immediate big issue.
some suggestions:
- you have a lot of
el?.
type code and have even a check at some point where you throw. if you don't want to have code run that depends on el
you could either add that code in an onMount
and just null assert, or what I like to do sometimes is to pass a function to ref like this
- you are manually diffing the event-listeners with syncEventListeners
, maybe this could be written more declaratively, like this. with your method you have more control over when exactly the event listeners are added tho.Solid Playground
Quickly discover what the solid compiler will generate from your JSX template
Solid Playground
Quickly discover what the solid compiler will generate from your JSX template
last point also counts for
syncWindowResize
o also
returning a callback in an effect in solidjs is not a cleanup-function, you should use onCleanup
for that instead.
not super sure what the intention is of that code
ye that code looks pretty fishyThanks! I tried to keep it as close to the React implementation as possible because I thought it might have a better chance of getting into the main project. And secondly I didn't want to spend a lot of time on it.
Good catch on the clean up code. I'll see if there's something to be done there. That looks like a fundamentally wrong aspect for solidjs.
I had to make some updates based on the info that @bigmistqke pointed out, but have been using the library without issue for a couple of weeks now.
I ran into some issues with the built version that I didn't see in the test app within the monorepo for the library. I thought it might be related to pnpm linking and I published a few alpha versions to npm while testing.
Ultimately I ended making two changes to get this working for running the dev server, local built server, and via npm package.
1. replacing
import Plotly from 'plotly.js/dist/plotly'
with import Plotly from 'plotly.js-dist-min'
in the index.tsx
2. Add a single tick delay (await new Promise(resolve => setTimeout(resolve, 0))
) in the factory when updating Plolty in factory.tsx
(2) seems like a bad solution, but I couldn't figure out why el
was undefined in this context since it's a reference and the first invocation is from the onMount
lifecycle method which happens once all initial rendering is done per the docs.
It seems that in some contexts (non-dev env) there is a single tick delay between mounting/rendering and when el
is set based on the reference.
Refs can also take the form of a callback function.Maybe it would be better for the initial
updatePlotly
call to be done in the ref function than in onMount
. 🤔
testing ....Based on the logs being printed it looks like it is possible for
onMount
to be called before the ref function has fired.This doesn't happen in the monorepo test app and Plotly throws an internal error if I try to to invoke it from the ref function in the case when the ref function fires before
onMount
even though the element is present.
GPT suggest a solution like this which I expect should work and will be more robust than a single tick await, but it's still icky.
Any other ideas on how to handle this?
Maybe el
can be a signal.hey agent!
oops think i misread ur msg
Hey!
I saw it and I think that's where I'm landing.
Making
el
a signal works for all cases except running the built (production) app locally when linked.
This works for dev and build.
"@ralphsmith80/solid-plotly.js": "0.0.1-alpha.7",
but this doesn't
"@ralphsmith80/solid-plotly.js": "link:/home/agentsmith/Workspace/solid-plotly.js/packages/solid-plotly.js/",
The dev server still works for the linked case so this is an edge case for sure, but it works in all scenarios if I use this.
I don't like the setTimeout option, but I understand why it works.i m git cloning ur lib rn and i will do a quick audit
Okay, if you want to wait for a minute I have it working now, but I would still appreciate some eyes.
I ran into some issues with the built version that I didn't see in the test app within the monorepo for the library. I thought it might be related to pnpm linking and I published a few alpha versions to npm while testing.https://github.com/stackblitz-labs/pkg.pr.new this is really handy for that.
Or I'll just give you a heads up after I push this change.
+ for ur next solid library, if u need a solid library setup without having to mess around with monorepos, i can recommend this starter https://github.com/solidjs-community/solid-lib-starter
GitHub
GitHub - solidjs-community/solid-lib-starter: SolidJS library start...
SolidJS library starter template. Use it to create your own solid package. - solidjs-community/solid-lib-starter
Yup, you shared that with me before. I've been setting up a pnpm monorepo and then using that to create the lib in the packages workspace. I find that to be a bit better for testing because I can have an apps workspace with an app for testing the library without needing a completely sperate project for testing.
that starter also has a dev thingy
w vite
Although, I did run into an issue because that utilizes linking.
it just doesn't use a monorepo to do it
exactly
monorepos are a paaaaain
it's a last resort for me
haha, there are definetly some nuances with it.
GitHub
solid-lib-starter/dev at main · solidjs-community/solid-lib-starter
SolidJS library starter template. Use it to create your own solid package. - solidjs-community/solid-lib-starter
but since it's in a single repo you don't need to rebuild your library everytime you update
these
{defer: true}
in the createMemos
are they some hackery to make ssr work?No, that was for Plotly. It all eventually calls this for Plotly.
Plotly.react(el, props.data, props.layout || {}, props.config)
But the should be one call like this updatePlotly(true, props.onInitialized, true)
which sets up event handlers for plotly the rest use this updatePlotly(false, props.onUpdate, false)
.
I can't remember if there was an issue if I didn't defer the effects, but as a matter of performance they don't need to be called intially.
-
I just pushed an update.
It's not a big change, but fixes the issues I was seeing on build and combines the memos with on
.
This is why I wasn't too sure about sticking with the dev app that comes with that.
https://github.com/solidjs-community/solid-lib-starter/blob/main/dev/vite.config.ts#L8
That's not a production version of the library and I can flip back an forth with the test app in a monorepo.ok couple of thoughts:
1. there must be a way to write the
updatePlotly
-effect without a dependency array
2. setting up the resize event handler, syncWindowResize
, could just be moved to onMount
since you are only doing it once
3. attaching and removing of the event handlers could be done more reactively too (you could use indexArray
or mapArray
for this)
4. you should use createEffect
or createRenderEffect
for side-effects, not memo (memos are for derivations, they return a signal with a computation)
what do you mean? i m not sure i m followingIt's referencing the local source files rather than the files produced from the build. i.e. the ones in the dist dir.
Also I can publish the package and update the package.json for the app in the apps workspace to reference that.
yes, to test out production i build the dev
but i only need to test out production 1% of the time
that's why i prefer without monorepo, but to each their own!
I like the simplicity of it, but I think I would have missed some issues I had without the setup. Or at least I would have needed to setup another project to test the package after it was published to npm. That's where I ran into some issues, but yeah it needed to be tested one way or the other.
(1) do you mean without this?
[data, layout, config, frames, revision],
exactly
it's not necessarily an issue, but it's just not very
solid
why are you removeUpdateEvents()
on cleanup? i think this is redundant since the element is removed when it is cleaned up anyway.
is typeof el.on !== 'function'
in attachUpdateEvents
some ssr hackery?So I was mostly copying the React library they had which is old in itself, but plotly is just a JS library and I'm invoking it's API based on the parameter you would need to track for invoking changes. Is there a better way to do that if the underlying library is not based on solid?
event handlers aren't automatically removed even if the underlying dom element is removed so this is just cleaning up what would turn into dangling refs.
It would be a small memory leak, but might not be a real problem in practice.
i don't think this is true
since the dom element is removed itself
It's what I remember from my old school JS only days, but I did check with GPT to confirm.
When you add event handlers in JavaScript, and then remove the corresponding DOM elements, the event handlers are not automatically removed. This can lead to "dangling references" that cause memory leaks because the event handlers are still retained in memory even though the DOM elements no longer exist. This problem is commonly referred to as a "zombie event listener."- Not that I expect GPT to know everything, but it does align with what I remember. It would be easy to test since there's an Event Listeners panel in dev tools. Yeah. I deleted the entire body of the page an the event listeners are still there.
a, but i think the clue is: the closure is a component and that one is also being detached
so since that is being cleaned up i don't think you will have dangling references
idk i could be wrong
Yeah, I'd have to think about how to test that.
i think this will not clean up the correct events tho, since these are 2 different references
Which reference is different?
This
const el = plotEl()
the eventhandler:
() => handleUpdate()
, you want to do el.on(updateEvent, handleUpdate)
and el.removeListener(updateEvent, handleUpdate)
instead
is this with the intent of providing default props to the component? or is this an api thingy?I don't think I need that. It was part of the initial GPT port from react, but since I'm using TS I don't think it's necessary.
made a pr w some proposed changes https://github.com/ralphsmith80/solid-plotly.js/pull/1
GitHub
solidify by bigmistqke · Pull Request #1 · ralphsmith80/solid-plotl...
made some updates to the code to follow more conventional solid patterns
changes:
not manually tracking/diffing update-event handlers, but use effects instead (syncEventHandlers becomes initEventH...
it wouldn't do anything in solidjs and it's not a convention that anybody else follows, so i removed it in the pr too. if you want to provide default props you can use
mergeProps
.I had a couple comments. Mostly points that I didn't even realize you could/should do with solidjs functions. That nesting in particular.
I'm going to run it through the use cases I was having issue with when using onRef myself to see if it holds up.
Otherwise I think it looks good. Looks like you mostly switched to relying on closures which was something I favored before going mostly React back in 2016.
It looks like this is working for all case except for the linked build I mentioned here. https://discord.com/channels/722131463138705510/1291807604284330044/1298763517721317437
It's probably not a big deal because the only reason you would do that is if you're linking for development purposes outside of the project repo and you want to produce a build for it in that context. I was linking for development because I wanted to get some cycles on it with the project I was working on before publishing which lead me to building it in that context to test.
I'm going to merge this in, but I still had one question just to confirm the synchronous execution is still working the way I expect.
https://github.com/ralphsmith80/solid-plotly.js/pull/1/#discussion_r1815005306
Thanks for the help btw!
ur welcome!
ugh ye linking can be a real pain in the ass
i am not capable of reproducing the bug tho, when i do
"@ralphsmith80/solid-plotly.js": "link:/../../packages/solid-plotly.js"
in apps/demo/package.json
it works on my end.Yeah, it works in the works space. If you create a separate project and link in that project you should see the error. I only ran into this because that's how I was using it since I hadn't published it yet, but I'm using the published 0.0.1 version now.
I'm seeing a build issue with this one too. https://discord.com/channels/722131463138705510/1277835110002987008/1277835110002987008
I don't know if it's similar, but the particles render in dev mode, but no in production.
I'm just about done with the basic of the project I'm working on. I could run without this, but I want to make it fancy.
lool ye i remember that code 🤣
tried it out in a separate folder and linking still works. i m on mac.
it takes a really long time to initially load the page tho, i think something to do with
[BABEL] Note: The code generator has deoptimised the styling of /Users/bigmistqke/Documents/GitHub/solid-plotly.js/packages/solid-plotly.js/dist/dev.jsx as it exceeds the max of 500KB.
. maybe plotly.js
should be a peer dependency, so that you don't bundle it with your own app.
a i do get an error when i build!I'm on windows using WSL which is just an unbuntu VM basically.
The babel error is because of the size of plotly. It is a peer dependency for the library. It's the application that gives that warning when running because of the size of plotly, but there's not much to be done about that. It's just a large library.
You get an error building? Is that when it's linked? I haven't seen that yet.
Ye the linking works for me in dev but it failed when building. That
trying to access on on null
type error. Had to work so couldn't investigate further, will have a look at it tomorrow.Okay, pretty sure that's the same issue I was seeing. I traced it through using dev tools.
It's coming from this line of code in the production file
Ze._container.enter().insert("div", ":first-child").classed("plot-container", !0).classed("plotly", !0),
.
The error is specifically at this point in the execution Ze._container.enter().insert("div", ":first-child")
.Based on the error and the JS calls it looks like it's trying to insert an element in the DOM, but getting an error the the el reference is not present yet. The
el
is in JS at this point for sure, but maybe it's not rendered just yet.
It should be because it's in the onMount function, but the error is related to accessing an element that not yet loaded in the DOM.
This also explains why the await new Promise(resolve => setTimeout(resolve, 0))
resolves the issue becuase it needs just one more tick to be loaded.Where is that namespaceURI coming from 🤔
Here in the minified plotly code.
Ze._container.enter().insert("div", ":first-child").
It's the insert
call specifically.
The element is there in JS. The only thing I can figure is that it's in memory, but not yet in the DOM.But where is the code that accessed that namespaceURI ?
I'm not following. That came from the error that was a result of the DOM api call.
Ye will have a look at it myself! Is going to be easier then trying to debug through you 😅
At a concert but can't stop myself from debugging lol
lol!
Missed this message but I saw the Github post and commented back. That's what I was thinking, but wasn't sure.