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

export client-side APIs from dom-expressions server.js, make them throw an error #345

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

trusktr
Copy link
Contributor

@trusktr trusktr commented Sep 1, 2024

Fixes:

Are there any implications that I have overlooked? If no one is currently importing Aliases/etc on the server (because they currently can't) then adding the exports should be harmless.

This PR goes along with

@trusktr
Copy link
Contributor Author

trusktr commented Sep 1, 2024

Still WIP, f.e. after this we get this downstream:

SyntaxError: The requested module 'solid-js/web' does not provide an export named 'classList'

I think for this we can do like:

export function classList(node, value, prev) {
  throw new Error("classList not supported on server side");
}

in server.js

For the type definition in server.d.ts we can use @deprecated to make it appear crossed out:

/** @deprecated not supported on the server side */
export function classList(
  node: Element,
  value: { [k: string]: boolean },
  prev?: { [k: string]: boolean }
): { [k: string]: boolean };

Then this way at least exports will be isomorphic and import will at least just work, and if anyone tries to use a client-only API on the server they'll get a runtime error. A runtime error in an API is a lot easier to handle than broken import and having to resort to await import() inside components which has a bag of worse problems.

I'll proceed with this, but let me know of there's any objection.

@trusktr
Copy link
Contributor Author

trusktr commented Sep 1, 2024

Alright, I updated it so that all client-side functions are exported from server.js; but they throw if called on the server side.

I tested with this version of dom-expressions, by running import('solid-js/html').then(console.log) in the node repl to show that html can be successfully imported.

I also verified that the errors are thrown for client-only APIs with import('solid-js/web').then(m => m.effect(() => {})).

I did this in a separate folder with updated solid-js linked into node_modules. Output sample:

❯ node
Welcome to Node.js v20.6.1.
Type ".help" for more information.
> import('solid-js/html').then(console.log)
Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 374,
  [Symbol(trigger_async_id_symbol)]: 366
}
> [Module: null prototype] { default: [Function: html] }
> import('solid-js/web').then(m => m.effect(() => {}))
Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 711,
  [Symbol(trigger_async_id_symbol)]: 703
}
> Uncaught:
Error: Client-only API called on the server side. Run client-only code in onMount, or run client-only component with <Show>.
    at Module.notSup (file:///Users/trusktr/src/solidjs+solid/packages/solid/web/dist/server.js:654:9)

I think it is nice to be helpful with the error message, but an alternative is to no-op (and let some other error happen instead).

@@ -57,7 +57,7 @@ export function dynamicProperty(props: unknown, key: string): unknown;
export function hydrate(
fn: () => JSX.Element,
node: MountableElement,
options?: { renderId?: string, owner?: unknown }
options?: { renderId?: string; owner?: unknown }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier did it

@@ -466,7 +466,7 @@ function insertExpression(parent, value, current, marker, unwrapArray) {
if (marker === undefined) return (current = [...parent.childNodes]);
let node = array[0];
if (node.parentNode !== parent) return current;
const nodes = [node]
const nodes = [node];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier did it

@trusktr trusktr marked this pull request as ready for review September 1, 2024 15:57
@trusktr trusktr changed the title export constants from dom-expressions server.js export client-side APIs from dom-expressions server.js, make them throw an error Sep 1, 2024
trusktr added a commit to lume/showcase that referenced this pull request Sep 1, 2024
…sions#345 so that we can use regular `import` statements for `lume` and remove the `defineElement` function wrappers
@trusktr trusktr marked this pull request as draft September 1, 2024 18:23
@trusktr
Copy link
Contributor Author

trusktr commented Sep 1, 2024

Converted back to draft, doing some testing to ensure that the build is working dependong on Solid Start ssr mode or not...

notSup as getNextMatch,
notSup as getNextMarker,
notSup as runHydrationEvents
};
Copy link
Contributor Author

@trusktr trusktr Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And are these accurate? Wondering if I missed anything. Maybe this is good enough to start with at least. The Aliases errors are gone when importing solid-js/html server-side for example, which is good enough for my case.

Copy link
Contributor Author

@trusktr trusktr Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all new functions never exported before for server, so it is non-breaking to make them errors I believe. However the question is, do you want any of them to be no-ops instead of error-throwing? I think errors are better for DX to inform people what not to do.

Also, any of these functions can be overriden to be no-ops over in solid-js, for example like some already are here:

https://github.com/solidjs/solid/pull/2269/files#r1740197787

…ports, and make client-only functions throw on the serverside
@@ -1,7 +1,19 @@
import { Aliases, BooleanAttributes, ChildProperties } from "./constants";
import { sharedConfig, root } from "rxcore";
import { createSerializer, getLocalHeaderScript } from "./serializer";
export { createComponent } from "rxcore";

export { getOwner, createComponent, effect, memo, untrack } from "rxcore";
Copy link
Contributor Author

@trusktr trusktr Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this list accurate? I based this off of web/src/core.ts, which I assume indicates which APIs are available server-side, but I did not include those that were already included here in server.js (f.e. mergeProps is defined here in server.js).

Goal is to bring server.js exports to closer parity with client.js exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, no, not accurate. The code is difficult to follow.

I see that the build output has code injected, that I need to avoid clashing with:

const isServer = true;
const isDev = false;
function render() {}
function hydrate() {}
function insert() {}
function spread() {}
function addEventListener() {}
function delegateEvents() {}
function Dynamic(props) {
  const [p, others] = splitProps(props, ["component"]);
  const comp = p.component,
    t = typeof comp;
  if (comp) {
    if (t === "function") return comp(others);else if (t === "string") {
      return ssrElement(comp, others, undefined, true);
    }
  }
}
function Portal(props) {
  return "";
}

Copy link
Contributor Author

@trusktr trusktr Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this list of exports? For example function render() {} is injected, and that is not exported in server.d.ts.

Scratch that. I was confused because of dom-expressions and rxcore indirection. Hard to follow, IDE intellisense has no idea what is happening (therefore me too lol).

Copy link
Contributor Author

@trusktr trusktr Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is confusing because it looks like the exports from dom-expressions get merged with the exports from web/server/index.ts into a single file.

I know what to do now...

@@ -1,7 +1,19 @@
import { Aliases, BooleanAttributes, ChildProperties } from "./constants";
import { Aliases, BooleanAttributes, ChildProperties, Properties } from "./constants";
Copy link
Contributor Author

@trusktr trusktr Sep 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import was missing, see further below in the file where Properties.has(prop) was being called, but Properties was not defined (TypeScript is not type checking these JS files).

@trusktr
Copy link
Contributor Author

trusktr commented Sep 1, 2024

Ok I think this is finally ready again.

@ryansolid
Copy link
Owner

Yeah just eyeballing this looks pretty good.

@ryansolid ryansolid changed the base branch from main to next September 9, 2024 21:37
@ryansolid ryansolid merged commit 71af9d6 into ryansolid:next Sep 9, 2024
1 check passed
@trusktr trusktr deleted the update-exports branch September 10, 2024 02:55
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

Successfully merging this pull request may close these issues.

2 participants