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

fix(solid-form): Add solid export condition #631

Merged
merged 9 commits into from
Jun 20, 2024

Conversation

Brendonovich
Copy link
Contributor

Solid relies on the solid export condition to perform a custom JSX transform depending on whether the code is being built for client or server, but @tanstack/solid-form only has dist/cjs and dist/esm, which are client-only since they import stuff like memo from solid-js/web (which is only available in client builds).

To fix this I've added tsc to the build of process of @tanstack/solid-form so that .js/.jsx files are emitted which solid can apply its custom JSX transform on. There's probably nicer ways of doing this that can integrate with vite but this is what I did to fix a similar problem for SolidStart.

Copy link

nx-cloud bot commented Mar 19, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 80035f2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@crutchcorn
Copy link
Member

@lachlancollins can you investigate this? We'd want this to integrate with Vite

Also CC @ryansolid if you have any guidance on how to do this (sorry for the ping Ryan)

@ryansolid
Copy link

This is pretty much correct. We want the source, but not necessarily the TypeScript so we tend to tsc it into dist folder. My only recommendation is build it into a .jsx entry file if there is any JSX anywhere in the solid implementation of the library. That way it will get processed properly.

packages/solid-form/package.json Outdated Show resolved Hide resolved
packages/solid-form/tsconfig.build.json Outdated Show resolved Hide resolved
@Brendonovich
Copy link
Contributor Author

My only recommendation is build it into a .jsx entry file if there is any JSX anywhere in the solid implementation of the library. That way it will get processed properly.

This is probably true but having the entrypoint be .js seems to be working fine for us

@@ -22,7 +22,7 @@
"test:lib": "vitest",
"test:lib:dev": "pnpm run test:lib --watch",
"test:build": "publint --strict",
"build": "vite build"
"build": "vite build && tsc -p tsconfig.build.json"
Copy link
Member

Choose a reason for hiding this comment

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

Leaving a comment here to remind @lachlancollins to investigate moving this into our Vite build if at all possible

Copy link
Member

Choose a reason for hiding this comment

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

@crutchcorn I've tweaked the config slightly so that it generates a flatter structure. AFAIK, there is no need for the dist/source folder to have its own types, since they are identical to those in dist/esm (but if not, it can be re-enabled by removing "declaration": false).

I agree it might be nice to have a direct TS->JS copy of the source built by @tanstack/config, I'll have a look!

@TheKnightCoder
Copy link

Is there anything blocking this from being merged? tanstack forms is unusable on solid without this

@crutchcorn
Copy link
Member

@TheKnightCoder I'd like to see our build tooling unified to solve this problem before we merge. @lachlancollins any chance you could take another look into merging into Vite?

Alternatively, if you think this fine to publish as-is for now and refine later, we can

@lachlancollins lachlancollins changed the title chore: add solid export condition to @tanstack/solid-form fix(solid-form): Add solid export condition Jun 19, 2024
@crutchcorn
Copy link
Member

Merging, per some research from @lachlancollins showing this should work now. Let us know if you run into additional problems around ESM/CJS support

@crutchcorn crutchcorn merged commit e9e0bab into TanStack:main Jun 20, 2024
2 checks passed
@Brendonovich Brendonovich deleted the solid-export-condition branch July 9, 2024 04:49
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.

[Feature?]: @solidjs/start should provide .d.ts
5 participants