solid icon indicating copy to clipboard operation
solid copied to clipboard

OnMount function is being called before elements are mounted

Open Nizarazo opened this issue 2 years ago • 16 comments

Describe the bug

Hi Everyone

I am struggling with an issue that need your help to solve it:

I see that OnMount function is being called while the ref element is still not initialized, this happened when I run the solid app using double click on index.html generated by "npm run build" using vite The issue isn't reproduced if I run npm start

Here is a snippet code showing the issue: const Slider: Component <SliderProps> = (sliderProps) => {

let sliderContainerRef: HTMLDivElement | any;

onMount(() => { console.log("onmount is called, sliderContainerRef:" + sliderContainerRef); } return (<div class={"slider-line"} ref={sliderContainerRef}>);

Results in console when running npm start: [vite] connecting... client.ts:53 [vite] connected. slider.tsx:153 onmount is called, sliderContainerRef:[object HTMLDivElement]

Results in console when double clicking on index.html: onmount is called, sliderContainerRef:undefined

Sleeping for a while solved the issue: onMount(async () => { while (sliderContainerRef == undefined) { await sleep(50); } console.log("onmount is called, sliderContainerRef:" + sliderContainerRef); });

Results in console when double clicking on index.html with sleep: onmount is called, sliderContainerRef:[object HTMLDivElement]

Your Example Website or App

Dont have

Steps to Reproduce the Bug or Issue

Please see description.

Expected behavior

I am expecting that the ref element should be initialized before OnMount is being called since OnMount: "runs after initial render and elements have been mounted. Ideal for using refs and managing other one time side effects."

Screenshots or Videos

No response

Platform

Windows 10 Chrome

Additional context

No response

Nizarazo avatar Sep 09 '22 12:09 Nizarazo

Have a see at #1199. ~~Solid 1.5 change the effect execute order~~

Sorry for the misunderstanding.

Genuifx avatar Sep 09 '22 14:09 Genuifx

I want to clarify #1199 is an initial misunderstanding. Effects execution order did not change in 1.5.

ryansolid avatar Sep 09 '22 14:09 ryansolid

@ryansolid Thanks for your reply.

  1. Is it guaranteed that onMount is called only after all html elements including ref elements is mounted/rendered?
  2. Can you tell me why running index.html by double clicking and running "npm start" would give different results as explained in the description?

Nizarazo avatar Sep 09 '22 14:09 Nizarazo

  1. Not if there are things that are conditional rendered. But in general onMount/createEffect run after all internal effects run which means both all initial DOM elements are created and attached.
  2. No idea, this makes no sense. I guess let me see you are using a vite template I gather? I wonder if something is weird at the mount point. npm start in the template looks like it aliases dev. Try running npm run serve. After build that might help us narrow it down to a dev vs non-dev thing.

ryansolid avatar Sep 09 '22 16:09 ryansolid

@ryansolid Issue is reproduced if running: "npm run serve" after "npm run build" while it isn't reproduced when running "npm start" meaning it is dev vs production thing, any idea what makes this different behavior?

Here is my package.json: "scripts": { "start": "vite --open", "dev": "vite --open", "build": "vite build", "serve": "vite preview" },

Nizarazo avatar Sep 09 '22 18:09 Nizarazo

Offhand no. I think I need a reproduction. I've never heard of this being reported before.

ryansolid avatar Sep 09 '22 18:09 ryansolid

  1. Can you tell me why running index.html by double clicking and running "npm start" would give different results as explained in the description?

Dumb question from me sorry. Won't double-clicking on the html file just open it as a file from your file system as opposed to serving it through a url such as http://localhost:3000?

I'd expect you need to start the server and browse to the relevant port instead of double-clicking the file. Or perhaps there is some other magic in play here?

Brendan-csel avatar Sep 09 '22 23:09 Brendan-csel

Double-clicking the html file wouldn't work.

lxsmnsyc avatar Sep 10 '22 02:09 lxsmnsyc

I mean it sounds like this issue is independent of that. Vite creates a dist folder. If you open that locally in the filesystem it will work without making a server. Chrome will open it up locally and open relative resources.

But it's beside the point. The issue here is with the prod build. But need a reproduction.

ryansolid avatar Sep 10 '22 02:09 ryansolid

Maybe it works on Mac but on Windows I get network tab errors about JS files being blocked. It may also depend on whether the vite config has some other "/" based public folder configured - meaning all the resource links are not relative to the html file.

Brendan-csel avatar Sep 10 '22 04:09 Brendan-csel

Hi

Thank you all for your response. @ryansolid : I am working to reproduce the issue with minimal code as possible, should I attach the source code here via github?

@Brendan-csel It is reproduced on windows 10, I will need to check it on mac to see if it is mac vs windows thing,

@lxsmnsyc double clicking the html file would give issue with CORS (JS files being blocked) since you need to start a server but this can be solved by three ways:

  1. You can make inline index.html by vite-plugin-singlefile package: https://www.npmjs.com/package/vite-plugin-singlefile
  2. You can run "npm run serve" which will start a server and run the index.html
  3. You can fetch a public folder via express server by: app.use(express.static('public')) where public is the folder name located inside your express project.

Please note I was able to reproduce the issue with the three methods above.

Thanks

Nizarazo avatar Sep 11 '22 08:09 Nizarazo

Stackblitz might be the easiest. But I can work from GitHub repo.

ryansolid avatar Sep 12 '22 08:09 ryansolid

Hi @ryansolid

Here is the github repo with main branch which reproduce the issue by the following steps:

  1. git clone to main branch https://github.com/Nizarazo/solid-js-issue
  2. open vs code and then open terminal
  3. lerna bootstrap
  4. cd solid-js-issue\packages\blender\client
  5. npm run build
  6. npm run serve
  7. Type the link http://127.0.0.1:4173/ in browser
  8. inspect element you see error due reference element still not being initialized:

ObjectsliderContainerRef: undefined[[Prototype]]: Object index.4f5de954.js:5 Uncaught TypeError: Cannot read properties of undefined (reading 'getBoundingClientRect') at E (index.4f5de954.js:5:1045) at index.4f5de954.js:5:1309 at de (index.4f5de954.js:1:10546) at Object.fn (index.4f5de954.js:1:10578) at Yt (index.4f5de954.js:1:11509) at ge (index.4f5de954.js:1:11452) at rt (index.4f5de954.js:1:10499) at Xt (index.4f5de954.js:1:10571) at xn (index.4f5de954.js:5:1263) at index.4f5de954.js:1:5491

With branch solid-js-fix there is a fix which is moving the folder solidjs-infra from root to blender/client/src with the fix you should see sliders.

Don't know why moving the folder to the src directory solves the issue, but I need the folder in root folder in order to have a reusable code.

Please let me know if you need any more info.

Thanks

Nizarazo avatar Sep 14 '22 10:09 Nizarazo

Hmm.. moving it making a difference suggests an interesting problem. Maybe we have 2 versions of Solid. The one for user effects is different than what is being imported by the JSX transform. Let me see if I can confirm that.

ryansolid avatar Sep 14 '22 11:09 ryansolid

@ryansolid any update about the fix?

Nizarazo avatar Sep 16 '22 07:09 Nizarazo

There are definitely 2 versions of Solid in your project and that is causing the problem. It looks like each package brings its own Solid version with it. What you want is for them to use the same version. Generally the lib I imagine would externalize solid-js. But in this situation where you aren't bundling the other one that's just how node_module resolution works. Hoisting deps might work but right now both are getting pulled in.

ryansolid avatar Sep 21 '22 04:09 ryansolid

@ryansolid Thanks for your reply.

To check the version of solid-js I run the command "npm list" it looks same version 1.5.6 on both locations:

  1. https://github.com/Nizarazo/solid-js-issue/blob/main/packages/blender/client/package.json
  2. https://github.com/Nizarazo/solid-js-issue/blob/main/packages/solidjs-infra/package.json

Please see the results below of npm list I marked the version of solid-js with bold: PS C:\p4client\ProAudio\dev_main\ProAudio\XPlatform\Common_Sources\HTML_Packages\packages\blender\client> npm list
[email protected] C:\p4client\ProAudio\dev_main\ProAudio\XPlatform\Common_Sources\HTML_Packages\packages\blender\client ├── @types/[email protected] ├── [email protected] ├── [email protected] ├── [email protected] -> .....\infra ├── [email protected] ├── [email protected] ├── [email protected] -> .....\solidjs-infra ├── [email protected] ├── [email protected] ├── [email protected] ├── [email protected] └── [email protected]

PS C:\p4client\ProAudio\dev_main\ProAudio\XPlatform\Common_Sources\HTML_Packages\packages\blender\client> cd .. PS C:\p4client\ProAudio\dev_main\ProAudio\XPlatform\Common_Sources\HTML_Packages\packages\blender> cd .. PS C:\p4client\ProAudio\dev_main\ProAudio\XPlatform\Common_Sources\HTML_Packages\packages> cd .\solidjs-infra
PS C:\p4client\ProAudio\dev_main\ProAudio\XPlatform\Common_Sources\HTML_Packages\packages\solidjs-infra> npm list [email protected] C:\p4client\ProAudio\dev_main\ProAudio\XPlatform\Common_Sources\HTML_Packages\packages\solidjs-infra ├── @types/[email protected] ├── [email protected] ├── [email protected] -> ...\infra └── [email protected]

So it is the same version used can you advice? Thanks

Nizarazo avatar Sep 23 '22 13:09 Nizarazo

I'm more interested in this:

[email protected]

lxsmnsyc avatar Sep 23 '22 14:09 lxsmnsyc

Yeah i tried matching versions and it didn't help either. But I see 2 copies of every function in the build output (the second set has a $1 attached to the names). This is a mono-repo/bundling issue, but I am not certain what a good solution looks like.

ryansolid avatar Sep 23 '22 15:09 ryansolid

export default defineConfig({
  plugins: [solidPlugin(), solidSvg()],
  build: {
    target: 'esnext',
    polyfillDynamicImport: false,
  },
  resolve: {
    dedupe: ['solid-js'], <----------------------
  },
});

https://stackblitz.com/edit/github-scrvmy?file=packages%2Fblender%2Fclient%2Fvite.config.ts

LiQuidProQuo avatar Sep 23 '22 18:09 LiQuidProQuo

I added the code change in vite.config.ts given by @LiQuidProQuo and it fixed the issue, thanks @LiQuidProQuo @ryansolid The question if this fix in vite.config.ts is considered as a good solution? or you would rather fix it somewhere else such as solid-js framework or lerna?

Nizarazo avatar Sep 28 '22 10:09 Nizarazo

Its definitely outside scope of the framework. Outside of the warning we give in dev there is nothing we can do. We didn't see warning in dev I think? Since Vite is bundling stuff differently. Lerna has the ability to hoist dev deps which is how I usually solve this. Have them install at the root. Otherwise Vite config seems fine I guess. Although I wonder if dedupe works well with different versions. Which is probably the main reason we wouldn't just put this config in tte plugin.

ryansolid avatar Sep 28 '22 14:09 ryansolid

@ryansolid If going with vite config solution is ok then this issue can be closed. Thanks

Nizarazo avatar Sep 30 '22 08:09 Nizarazo