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

bundle solid-start/entry-client #1034

Closed

Conversation

AlexErrant
Copy link
Contributor

@AlexErrant AlexErrant commented Aug 28, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)
  • Build related changes

What is the current behavior?

Solid-Start is distributed as Typescript, leading to issues like #1008 and #255 This is not recommended.

What is the new behavior?

This PR creates a new solid-start/entry-client "package", similar to web, store, h, etc. in SolidJS. (Really I just looked at SolidJS and was like ctrl-c ctrl-v.) I only did entry-client for now to solicit feedback - I imagine the other folders of start to be more or less the same type of change. If the maintainers approve, I'll do the other dirs too.

This PR introduces breaking changes. Users on "moduleResolution": "node", will need to make changes like

-      import mount from "solid-start/entry-client/mount";
+      import mount from "solid-start/entry-client/dist/mount";

I'd argue this is a good thing as it forces Solid (and users) to think about the API, buuuuut I'd get it if that means we need to wait for 0.4 before this PR gets any traction. Also, since I'll be moving nearly every file in /start (to a nested /src folder), it may introduce conflicts with other PRs/branches. IMO this is a reason to get this done sooner rather than later, but choosing the right time may be important. Sorry if this ruins your timing, but I'm tired of getting ts errors in my node_modules 😭

@AlexErrant AlexErrant force-pushed the dist_solid-start/entry-client branch from 3793427 to d61d9d9 Compare August 28, 2023 21:07
Comment on lines +16 to +35
declare global {
interface Window {
router: {
navigate: (to: string, options?: Partial<NavigateOptions>) => Promise<boolean>;
push: (to: string | URL, options: Partial<NavigateOptions>) => void;
update: (body: string) => Promise<boolean>;
router: EventTarget;
location: () => LocationEntry;
};
}
interface StartRoutes {
$: {
params: any;
data: any;
};
}
interface Route {
"/notes/[note]": "/notes/[note]";
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was generated from Router.tsx. It's used by the tsconfig via "types": [ ..., "solid-start/router", which afaik only accepts .d.ts files (or whole projects).

In actuality all entry-client needs is this here declare global, but I'm leaving it in Router.tsx for now for easy review. Could easily move it elsewhere if requested!

This reverts commit 3efacdd.
since they're in the base tsconfig, which is extended by `entry-client/tsconfig.json`
Comment on lines +1 to +7
{
"name": "solid-start/entry-client",
"module": "./dist/index.jsx",
"types": "./dist/index.d.ts",
"type": "module",
"sideEffects": false
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cf. https://github.com/solidjs/solid/blob/main/packages/solid/html/package.json

I'm not including main because I believe we require Node18.... I don't know nearly enough about Node to know if that's good enough justification but we're not using a "real" bundler like Rollup. Maybe later?

@ryansolid
Copy link
Member

Truthfully.. we're sort of completely changing all of this in 0.4.. like completely changing the entry imports etc.. so I will take this under consideration but no action at the moment.

@AlexErrant
Copy link
Contributor Author

Coolbeans, more than happy to make PRs if/when you get to that point!

@AlexErrant AlexErrant closed this Aug 29, 2023
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