solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.05k stars 914 forks source link

Requiring "solid-js/web" on electron (with nodeIntegration) makes it think I'm on the server #1102

Closed mauricioszabo closed 1 year ago

mauricioszabo commented 2 years ago

Describe the bug

I want to use SolidJS in an Electron. Using require('solid-js/web').isServer returns true, which means I can't use render and other web-related code.

I can work-around by requiring solid-js/web/dist/web.cjs but seems fragile...

Your Example Website or App

https://github.com/mauricioszabo/solid-electron-error

Steps to Reproduce the Bug or Issue

Inside an electron app with nodeIntegration: true try to require solid-js/web. The contents of isServer is true

Expected behavior

I expect require('solid-js/web') to always require "web" stuff, or at least have something like require('solid-js/browser') that always points to browser, and require('solid-js/server') that always points to server

Screenshots or Videos

No response

Platform

Additional context

No response

edemaine commented 2 years ago

This is the normal behavior in Node. Currently, on Node, importing solid-js itself gives you an SSR build which has no reactivity. So solid-js/web can't "just work".

Importing directly from solid-js/web/dist/web.cjs or solid-js/web/dist/web.js (ESM) is one current workaround. Another way to set a Node condition of "browser" (via -C or NODE_OPTIONS), but I'm not sure whether this would interfere with Electron.

I agree that this situation is pretty annoying for using reactivity on Node, but we need an actual proposal for behavior that still enables SSR work.

mauricioszabo commented 2 years ago

The problem is that I can't use templating libs like h because the also require solid-js/web :(

mauricioszabo commented 2 years ago

But I actually don't get it - why can't Solid have two different requires, one for SSR and one for Browser? Why they need to be the same path? Is there a reason for that?

edemaine commented 2 years ago

I think the reason is "being isomorphic". Usually you have one file define a component, and that file needs to be loaded on both client and server for SSR. And it needs to import ... "solid-js" and do different things on each side.

I think Node conditions are a good way. I just don't like that you need to set a condition of "browser" to get server-side reactivity. Personally I'd prefer if you had to set a condition of "ssr" to opt-in to SSR, and the default Node import would just give you reactivity out of the box. But we might need to find something more backward compatible, like a "reactive" condition to tell Node to just load the client build. (I assume you don't need SSR in an Electron context.)

mauricioszabo commented 2 years ago

I think I may be missing something: this code does not work on NodeJS, for example (pure node, no Electron):

const h = require('solid-js/h');
const s = require('solid-js/web');
function A() { 
  return h('div', {}, 'Hello, world!') 
}

s.renderToString(A)

It fails with "Uncaught TypeError: Cannot read property 'has' of undefined". I found that the trouble is that solid-js/h actually loads solid-js/web and then uses web.SVGElements - which is undefined...

Which is to say: as of now, h(...) is not working, so the API does not seem "isomorphic" really... I'm not sure how to proceed from here...

edemaine commented 2 years ago

It seems that Electron might define a Node condition "electron". If true, perhaps Solid could be modified to export the client build in this case, as I can't imagine it being useful to do SSR in Electron?

Genuifx commented 2 years ago

You can split your UI and backend in electron, simulating the network's C/S artfs. then use solid at your UI part.

ryansolid commented 2 years ago

Personally I'd prefer if you had to set a condition of "ssr" to opt-in to SSR, and the default Node import would just give you reactivity out of the box. But we might need to find something more backward compatible, like a "reactive" condition to tell Node to just load the client build. (I assume you don't need SSR in an Electron context.)

Me too but I wanted it to be consistent with legacy. Pre esm and export conditions having the browser field was the pattern. Which makes sense. Package.json was built for npm, ie node.

mauricioszabo commented 2 years ago

So, after talking in Discord, I decided to make some experiments with a greenfield Electron app. It actually doesn't work, even if I "patch" solid/h to require the browser target - the UI renders, but it does not react in any way: https://github.com/mauricioszabo/solid-electron-error

To run, just install packages and run with npm start. The relevant code is on the view.js file.

mauricioszabo commented 2 years ago

Also, updating what we were discussing on Discord here: I'm trying to use SolidJS to make some changes in an Atom plug-in, so I can't really change NODE_OPTIONS or things like that (because that's handled by Atom itself). I also can't use JSX because the plug-in is written in ClojureScript, and there's no way to return a JSX content inside ClojureScript, unfortunately...

ryansolid commented 2 years ago

So, after talking in Discord, I decided to make some experiments with a greenfield Electron app. It actually doesn't work, even if I "patch" solid/h to require the browser target - the UI renders, but it does not react in any way: https://github.com/mauricioszabo/solid-electron-error

I haven't pulled this down but if I were to guess it's because solid-js/web references solid-js directly. So even with the client version of web we are getting the server version of solid's core. The server version is non-reactive which would explain no updates. Short of using conditions, you'd need to alias all the references. This is how we landed on conditions in the first place. Trying to get this stuff to play nice for isomorphic SSR was a pain. As it turns out going the opposite direction is similarly so.

mauricioszabo commented 2 years ago

you'd need to alias all the references How do I do that?

Also, is there a way to make Electron behave as if it's always a web version? I'm not sure why one would want to use SSR on Electron...

ryansolid commented 2 years ago

you'd need to alias all the references

How do I do that?

Depends on the bundler being used.

Also, is there a way to make Electron behave as if it's always a web version? I'm not sure why one would want to use SSR on Electron...

But this I think we can do. It has to be how others solve this because we aren't the only ones with this issue for sure. Maybe not export conditions but stuff like the browser field predate this. Targets like workers sometimes need similar consideration. My guess is electron sets node which is why we don't get the client version which is the default or it uses the main field export directly which would also do this. The latter I imagine is more likely so, let me see what others do.

Hmm.. this is dated, using webpack with aliasFields https://medium.com/@dylanavery720/marko-marko-3-electron-webpack-84e3edbe90cc

This is documentation from webpack on how to set conditions: https://webpack.js.org/configuration/resolve/#resolveconditionnames. In which case we'd want browser. In the list.

Sorry lack of familiarity with electron specifically and the build process. But most bundlers have the ability to set these things.

edemaine commented 2 years ago

Me too but I wanted it to be consistent with legacy. Pre esm and export conditions having the browser field was the pattern. Which makes sense. Package.json was built for npm, ie node.

Yeah, makes sense. Another option would be to publish as a totally different NPM package. solid-js could remain the SSR-capable version, while @solid.js/solid or something like that (@solid.js/reactive? @solid.js/nossr?) could be the always-reactive never-SSR version. Then we wouldn't have to rely on Node conditions... I think this would be great for using Solid reactivity into Node in general (without GUIs) and it would fix this particular problem easily too.

ryansolid commented 2 years ago

@edemaine That'd be fine for the reactive stuff. Like people want to just use reactivity.

But like this case here we actually want web as well. So references in web have to change as well. So I don't think that actually helps a ton here, or testing envs or any of the typical Solid workflows.

edemaine commented 2 years ago

Sorry, to clarify, I meant you could import ... from:

All the subdirectories would work as before, but with only the browser (and dev) exports that solid-js currently has. You'd just npm install @solid.js/solid instead of npm install solid-js.

Of course, everything would break down if you installed both packages solid-js and @solid.js/solid, because then you'd get two copies of Solid core. Oh, and there's the rub: if you use a library that imports solid-js, you'd get exactly this. So while this approach would work in isolation, it won't work with any code that uses libraries. ☹

mauricioszabo commented 2 years ago

But this I think we can do

If possible, it would help me a lot. My case is quite complicated because I'm using Solid with ClojureScript. It uses closure compiler, which as far as I know don't handle NPM too well, but I'm running over Shadow-CLJS which do handle rewriting of NodeJS requires. I tried some configs, but probably did something wrong because I could not make it work...

ryansolid commented 2 years ago

Oh I see so this is more than just straight Electron. Because that article I linked from 2017 I imagine would still work today. But if webpack isn't even involved and we we are talking about some bundling/building that doesn't have any of these sort of aliasing. Looking at common config though it looks like renderer is set with browser target at least in starters like: https://github.com/ahonn/shadow-electron-starter/blob/master/shadow-cljs.edn

So I would have hoped this would just find the browser fields. Hmm.

carlosqsilva commented 2 years ago

don't know what bundler are you using, but I put together this esbuild plugin for my electron application with solid-js, you could use this article (shadow-cljs with webpack) as a reference, and make it works with esbuild, hope it helps


const { parse } = require("path");
const { readFile } = require("fs/promises");
const { transformAsync } = require("@babel/core");
const solid = require("babel-preset-solid");
const ts = require("@babel/preset-typescript");

const defaultOptions = {
  hydratable: true,
  generate: "dom",
};

const regexMap = {
  "solid-js": /(?<=solid-js\/).*$/gm,
  "solid-js/web": /(?<=solid-js\/web\/).*$/gm,
  "solid-js/store": /(?<=solid-js\/store\/).*$/gm,
};

/** @type {() => import('esbuild').Plugin} */
function solidPlugin(options) {
  const pluginOptions = Object.assign({}, defaultOptions, options);

  return {
    name: "esbuild:solid",

    setup(build) {
      build.onResolve({ filter: /solid-js/ }, ({ path, resolveDir }) => {

        let newPath = require.resolve(path, {
          paths: [resolveDir],
        });

        if (path === "solid-js/web") {
          newPath = newPath.replace(regexMap[path], "dist/web.js");
        } else if (path === "solid-js/store") {
          newPath = newPath.replace(regexMap[path], "dist/store.js");
        } else {
          newPath = newPath.replace(regexMap[path], "dist/solid.js");
        }

        return {
          path: newPath,
        };
      });

      build.onLoad({ filter: /\.tsx$/ }, async (args) => {
        const source = await readFile(args.path, { encoding: "utf-8" });

        const { name, ext } = parse(args.path);
        const filename = name + ext;

        const { code } = await transformAsync(source, {
          presets: [[solid, pluginOptions], ts],
          filename,
          sourceMaps: "inline",
        });

        return { contents: code, loader: "js" };
      });
    },
  };
}

module.exports = { solidPlugin };

``
ryansolid commented 1 year ago

This section suggests webpack uses electron condition with either browser or node so we should already be good here: https://webpack.js.org/guides/package-exports/#target-environment.

Would love an update or confirmation. But looks like we should already be setup right.

ryansolid commented 1 year ago

Closing as stale.