solidjs / solid-router

A universal router for Solid inspired by Ember and React Router
MIT License
1.13k stars 143 forks source link

Unable to use `<Router />` #294

Closed bpevs closed 7 months ago

bpevs commented 1 year ago

Describe the bug

My env is Deno + Solid via esbuild. So I'm not entirely certain if this bug is totally just due to context. Just submitting because I already have a solution/workaround that seems fairly tame.

But basically, when trying to use a bare app with <Router />, I'm getting TypeError:

render(
  () => (<Router><App /></Router>),
  document.body
);
[Error] TypeError: node.addEventListener is not a function. (In 'node.addEventListener(name, handler)', 'node.addEventListener' is undefined)
    handleError (main.js:610)
    runUpdates (main.js:407)
    createRoot (main.js:53)
    render (main.js:997)
    Global Code (main.js:3943)

I'm finding this happens here: https://github.com/solidjs/solid-router/blob/main/src/integration.ts#L5 is window.addEventListener overridden by this? -> https://github.com/ryansolid/dom-expressions/blob/main/packages/dom-expressions/src/client.js#L117

That code is only used in two places [1] [2], and if I replace the bindEvent usage with the named versions of addEventListener, it seems to work as expected. Not sure if this is losing functionality by not using the overridden addEventListener, though:

notify => {
  window.onpopstate = () => notify()
  return () => window.onpopstate = null
}

Your Example Website or App

I can make an example app if necessary, but it's kind of a pain in the ass

Steps to Reproduce the Bug or Issue

import into project via import { Route } from "npm:@solidjs/router"

Deno build config is something like this:

{
    plugins: [
        denoResolver,
        solidPlugin({
          solid: { moduleName: 'npm:solid-js/web'  } }) as any,
        denoLoader
    ],
    entryPoints: [srcPath ? srcPath + "index.ts" : './src-www/index.tsx'],
    outfile: srcPath ? srcPath + "dist/main.js" : './src-www/dist/main.js',
    bundle: true,
    format: 'esm',
    target: ['chrome99', 'safari15'],
    treeShaking: true
}

Expected behavior

doesn't throw TypeError

Screenshots or Videos

No response

Platform

Additional context

No response

ryansolid commented 1 year ago

That's client only functionality which suggests you are getting the browser version of Solid instead of the server version. We use/compile package.json export conditions in order to get the right code. I'm not sure how something similar would work in Deno, but the "server" or "deno" condition is what is desired here. I'm not clear which package is getting the wrong thing but none of the event handler stuff should be running on the server.

bpevs commented 1 year ago

errrr sorry sorry I was not clear.

I'm definitely not using it on server; using Deno + esbuild to compile client code (so solid and solid router are both being run in a browser context); browser code is what I'm looking for here. This is an error I'm seeing in my browser console. mmmm lemme try and make a bare repo

bpevs commented 1 year ago

Gist repo recreating issue:

https://gist.github.com/bpevs/8aa43ff2c0f6e2632b204b0988fd2417#file-index-tsx

AirBorne04 commented 11 months ago

did you try to add platform to esbuild setting, even though it should be browser by default (not sure in serve though) check api here -> https://esbuild.github.io/api/#platform

bpevs commented 11 months ago

hmmm so adding platform as 'browser' or 'neutral' doesn't solve this. But yeah if this issue is just from resolving as node code instead of browser code, seems like maybe I'm really just having an issue with one of https://github.com/lucacasonato/esbuild_deno_loader or deno's npm: resolver, and that this behavior is just a symptom of that.

AirBorne04 commented 11 months ago

@bpevs I also checked the platform, did not change anything, adding a isServer print in console confirmed we do have browser code running. Next I checked the Stack trace and realized that the bind Element is calling addEventListener on the window object. That actually got overwritten by Solid-js addEventListener but that was not expected by the router. Quick check by throwing the example app into astro -> no issue there. Means there is something funky in the build setup that elevates the Solid-js functions to the global scope and that is not intended. Last check was using the deno adapter in astro which got rendered nearly correct, but received an error because server side code is run in the browser. This is a adapter issue though.

ryansolid commented 7 months ago

I don't know what to do with this one. It has gone stale now. We aren't doing anything special to window object. I'm glad it got narrowed down but I don't think without concrete examples there is much to be done on our side.

bpevs commented 7 months ago

@ryansolid oh yeah sorry I haven't really been trying too hard on it, since I have a workaround. But I am curious about what dom-expressions addEventListener is doing, though? Since I see that code attached as the global window.addEventListener, rather than what I'd expected ([native code])?

ryansolid commented 7 months ago

It's odd. I just made a wrapper so I can handle event binding and delegation. But it is just an in-scope function and not something we bind to window. It just gets exported. Then the compiler imports it and calls it. I guess that overrides global though. I didn't realize it would do that. Could look at renaming the helper. It would be a breaking change.

bpevs commented 7 months ago

Hmmm I'm assuming this is not happening in other build contexts, since things would be broken. But the esbuild and solid dependencies of my example are out of date. Lemme try to use babel-preset-solid more directly and see if I can't figure out what'd happening.

bpevs commented 7 months ago

Soooooo..... uhhhhhhhhhh..... I figured it out. It was dumber than I thought 😅. Your comment that the addEventListener was not supposed to overridden globally tipped me off.

For my esbuild, I was using format: "esm", which does not create an iife, and then running it in a script tag without a module attribute, causing the bundled/exported addEventListener to execute on the global scope (overriding the window).

In-host-project fix is to either: a. Use format: "iife" in build options b. Run script with type="module": <script src="dist/index.js" type="module"></script>

I updated the gist, if anyone runs across this and needs an example: https://gist.github.com/bpevs/8aa43ff2c0f6e2632b204b0988fd2417

Figuring a place to put a catch/warn somewhere could be helpful, but I'd consider this to be more incorrect usage than a bug, so probably making a breaking change in Solid would be a bad idea in this case?

Thanks for the comments that helped me distinguish what was intended! 🎉