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

element.ownerDocument.documentElement is null onMount in linked package #378

Open
bigmistqke opened this issue Oct 26, 2024 · 19 comments
Open

Comments

@bigmistqke
Copy link
Contributor

bigmistqke commented Oct 26, 2024

With the following snippet:

export const BugCheck =  () => {
  const [documentElement, setDocumentElement] = createSignal<HTMLElement>()
  return (
    <pre
      ref={element => {
        onMount(() => setDocumentElement(element.ownerDocument.documentElement))
      }}
    >
      {documentElement() === null ? 'NULL' : documentElement()?.innerHTML}
    </pre>
  )
}

We get different results in between dev and build when linking the package into a solid app:

Screenshot 2024-10-26 at 15 47 15

element.ownerDocument.documentElement is defined in dev mode, but is null when build.

When delaying it by a frame with requestAnimationFrame we do get the expected result:

Screenshot 2024-10-26 at 15 53 41

When inlining it directly in the app, it also does not give an error, so it's a quite surprising bug. Afaik the bug also does not seem to show up when downloading the published library, only during linking (am i correct in this, @ralphsmith80 ?)

a minimal reproduction of the bug

This bug came apparent during development of solid-plotly.js as plotly-js apparently makes use of this during initialisation.

@ralphsmith80
Copy link

That's correct @bigmistqke. It's only an issue when linking.

@titoBouzout
Copy link
Contributor

@ralphsmith80 do you have reproduction steps with the repo mentioned above, I did all the install and npm link dance but I couldnt get it to work

@titoBouzout
Copy link
Contributor

@bigmistqke wrote me in discord and helped me to reproduce, I will be investigating this, thanks!
leaving here the steps (I was using npm instead of pnpm and stuff just didnt work)

git clone
cd lib && pnpm install && pnpm build
cd ../app && pnpm install && pnpm build && pnpm serve

@ralphsmith80
Copy link

Perfect. I was going to clone it and test it myself since I didn't put this repo together for the issue. Figured it was a diff between npm and pnpm.

@titoBouzout
Copy link
Contributor

titoBouzout commented Nov 18, 2024

@ralphsmith80 the issue is that the build includes two versions of solid.

If you set NODE_ENV=development the console warns about two different versions of solid. Another way to check for this is to not minify the output with the vite option build.minify:false and then skim over the code looking for duplicate functions. I can see runUpdates and runUpdates$1 , duplicate functions show up with $1 in the name.

Looking into node_modules/solid-js/package.json I see both versions match. So I suppose that for whatever reason vite it's including it twice. I do not use vite myself, so can't really tell what's going on to suggest a fix.

I have started this thread in solidjs discord asking if anyone knows how to fix this
https://discord.com/channels/722131463138705510/723573901862371449/1308059822138069054
https://discord.gg/solidjs

@titoBouzout
Copy link
Contributor

Seems like app is using solid-js from
app/node_modules/solid-js
and "lib" from
lib/node_modules/solid-js

It has been commented in the linked thread that a "pnpm workspace" can be used to deal with this issue https://pnpm.io/workspaces

@ralphsmith80
Copy link

@titoBouzout, @bigmistqke was providing me a bunch of support when I was building a couple of libraries for solidjs, but if the issue you are seeing is two version of solidjs then I don't think the minimum example is capturing the issue.

In my own case I'm using the workspace linking and I don't see the multiple version issue, which I was seeing on the first library I was fixing so I am familiar with that occurring and I resolved it in my own cases.

Actually, you can see the issue in the solid-js wrapper for plotly.js at solid-plotly.js. It's a small repo so pretty minimal in itself.

It's a pnpm monorepo. It has a solid-plotly.js package which is the solidjs library that wraps plotly and you can see the code here which required a single tick await to fix the issue element.ownerDocument.documentElement is null onMount in linked package.

To test this there is an apps/demo application that uses a pnpm workspace link to link to the solid-plotly.js package here. This app renders a few different solid-plotly.js charts to test different configs, but you can comment them all out except for one so see the difference between running the built version and the dev version.

To see the issue you need to comment out that single tick await I pointed out above. Just make sure you rebuild the package so the change is picked up in the demo app.

@titoBouzout
Copy link
Contributor

Ok, but can you please provide a step-by-step instruction into how to reach a page opened in the browser that leads to such an issue being shown

@ralphsmith80
Copy link

@titoBouzout it's just a single page App.tsx that renders the charts together. You should only have to open the app pnpm dev and you should be there.

@titoBouzout
Copy link
Contributor

ok, I will try it when I make a bit of time, just that I do not use vite nor pnpm, so I have problems to reproduce what you expect, if you can guide me with some commands and "edit this line on this file after you install X" that would be very helpful

@ralphsmith80
Copy link

Bah! You won't be able to reproduce it with the monorepo for the reasons you noted above. It is a pnpm monorepo. It's only an issue if you link to it from another repro that's not part of the mono repo. I'm sure that why @bigmistqke provided the minimum repo example with linking the way he did.

I just tested the repo provide by @bigmistqke myself and I think your use of NODE_ENV=development is incorrect and doesn't help with solving the root cause.

I say that because you're using it when producing a production build (NODE_ENV=development pnpm build) which is incorrect. You can pass that when running the dev server pnpm dev and you won't see that issue, or you can run the production build pnpm build && pnpm serve and you won't see that issue. But if you specify it's a development environment while producing a production build you will see that issue, but that's an invalid configuration.

@titoBouzout
Copy link
Contributor

@ralphsmith80 I am with you. I said I do not use vite or pnpm personally, but I am willing to use any of both, and even change my environment variables to be able to reproduce your issue.

So if you can, please, can you give a list of commands, that I can follow (even if that involves editing npm packages at a file/line or whatever) that are reproducible of the problem you are seeing? That would be helpful, that's all I am saying.

I have run the reported "issue" and my conclusion is that there are 2 versions of solid running at the same time. (regardless of my configuration)

So, if you think I am mistaken (which is very likely), can you please provide me a list of procedures that I can follow to reproduce the issue. Thanks!

@ralphsmith80
Copy link

@titoBouzout, here's a repository that creates the error.

https://github.com/ralphsmith80/solid-link-issue

  1. I created it using the TS template from the solid site.
  2. Next, I installed the required per deeps.
    "plotly.js": "^2.35.2",
    "solid-js": "^1.9.3",
    
  3. Finally, I linked the local repo of my library. The path maybe different on your machine.
    "@ralphsmith80/solid-plotly.js": "../solid-plotly.js/packages/solid-plotly.js"

When I run pnpm dev I see no issues. When I run pnpm build && pnpm serve I see the issue as @bigmistqke described and I do not see the multiple instances of solidjs error/warning.

@titoBouzout
Copy link
Contributor

Thanks, I will check it out later today

@titoBouzout
Copy link
Contributor

I suspect that if you read the generated js file you will find out that solid functions are duplicated, but we will see

@titoBouzout
Copy link
Contributor

titoBouzout commented Dec 3, 2024

pnpm dev works
chrome_KE3HC5KRaT

pnpm build && pnpm serve doesn't work, multiple versions of solid included
chrome_BUc0ABbTK2

This is a bundler issue not a solid/dom-expressions issue.

I honestly don't know how to help you and I would say this is "expected" (like in this doesn't happen in the real world because people install the packages instead of linking them).

I also do like you for developing, link my packages, but when I get to the time of building for shipping, I install them (as it should be).

Anyway, the main problem seems to be that your machine is not setting NODE_ENV=development so you fail to see this message
chrome_QH6ehGg2gA

Why pnpm dev works and pnpm build && pnpm serve doesn't work is an interesting question, what has been suggested was to use a monorepo. I do not use monorepos myself, but what I know is that if I am building for production, then I use installed packages not linked packages. So theres that.

Also, please, as I said before if you can include a list of commands to reproduce your error, that would be much better.

This line on the package.json
"@ralphsmith80/solid-plotly.js": "../solid-plotly.js/packages/solid-plotly.js"
should be
"@ralphsmith80/solid-plotly.js": "link:../solid-plotly.js/packages/solid-plotly.js"
else that wont work on my system

@titoBouzout
Copy link
Contributor

If you use a Mac you can repro with the following command NODE_ENV=development && pnpm build && pnpm serve

@titoBouzout
Copy link
Contributor

For clarification element.ownerDocument.documentElement is null onMount because there are 2 versions of solid running at the same time, so one is not aware of what the other version is doing, so these kinds of errors are expected.

@ralphsmith80
Copy link

"@ralphsmith80/solid-plotly.js": "link:../solid-plotly.js/packages/solid-plotly.js"

Right, I just used pnpm link, but manually updated the package.json and it missed that.

If that's the issue so be it. I only used linking like this because I was trying to test the library more thoroughly before publishing it. Not because that's how I would build the production app that uses the library. In the end I published it and just crossed my fingers which worked.

Since it worked in the context of a mono-repo using linking, but no outside the context of a monorepo I didn't catch why there would be a difference, but if there are in fact two instances of solid.js in that case then it makes sense why that would happen.

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

No branches or pull requests

3 participants