update dom-expressions, solid-js/web, solid-js/html, solid-js/store to make the exports isomorphic #2269
Conversation
|
Pull Request Test Coverage Report for Build 10658254921Details
💛 - Coveralls |
| // Types for these come from dom-expressions/src/server.d.ts | ||
| // These override the functions from dom-expressions that throw on the serverside. | ||
| export function render() {} | ||
| export function hydrate() {} | ||
| export function insert() {} |
There was a problem hiding this comment.
These functions override the error-throwing functions that are now exported from dom-expressions, making this particular set of functions be no-ops instead of throwing errors.
All the other newly-added functions in dom-expressions (see here) continue to throw errors if they are not overriden here, which is non-breaking.
Removing the set of functions here could be breaking if anyone is already depending on them being no-ops, but the new functions added to dom-expressions/src/server.js never existed there or here for server code, so the introduction of those new error-throwing functions cannot be breaking because no one used them before.
If desired, those additional functions in dom-expressions can be overriden here to make them no-ops instead of error-throwing.
There was a problem hiding this comment.
This seems fine to me and honestly I think I'm ok even making them throw if called. I don't see any reason why these should ever be called on the server with a server build.
| @@ -37,7 +37,7 @@ | |||
| "babel-plugin-jsx-dom-expressions": "^0.38.5", | |||
| "coveralls": "^3.1.1", | |||
| "csstype": "^3.1.0", | |||
| "dom-expressions": "0.38.5", | |||
| "dom-expressions": "https://gitpkg.vercel.app/trusktr/dom-expressions/packages/dom-expressions?update-exports", | |||
There was a problem hiding this comment.
This is a quick way to install a package from any github repo, including sub-folders, useful for testing.
- update dom-expressions version before merge
There was a problem hiding this comment.
All tests are passing. Anything I may not have thought of that needs testing?
trusktr
changed the title
…d, marking the additional export as not supported on the server side
trusktr
changed the title
| @@ -103,6 +117,10 @@ export function createMutable<T>(state: T | Store<T>): T { | |||
| return state as T; | |||
| } | |||
|
|
|||
| export function modifyMutable<T>(state: T, modifier: (state: T) => T) { | |||
| notSup(); | |||
| } | |||
There was a problem hiding this comment.
Should this throw? Or should it be a no-op? I think that a no-op could be misleading, f.e. someone would think it worked, then log values only to see they didn't change.
There was a problem hiding this comment.
To be fair a lot of these methods still work on the server but just don't do anything. Like modifyMutable being missing is an oversight and should just function the same as in the client.
Depends on:
Summary
This PR will make exports between client and server match ("isomorphic"), to stop people from running into build errors.
We will update the dom-expressions version in this PR once this dom-expressions PR is merged:
The dom-expressions PR adds a version of client-side APIs to server.js that throw when they are used on the server side. This bring the exports of server.js closer to parity with client.js so that we can avoid issues like:
htmlfunction in Solid Start app solid-start#1618awaitdoes not work with libraries when enabled (and is not supported out of the box) solid-start#1614Furthermore we update
solid-js/storeto also have matching exports between server and client.How did you test this change?