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

Add SolidJS integration #3607

Merged
merged 20 commits into from
Sep 13, 2024
Merged

Add SolidJS integration #3607

merged 20 commits into from
Sep 13, 2024

Conversation

stefanmaric
Copy link
Contributor

@stefanmaric stefanmaric commented Jun 9, 2024

This is an attempt to fix the setup issues encountered by @Nvos on #3327

Copy link

changeset-bot bot commented Jun 9, 2024

🦋 Changeset detected

Latest commit: 97601bc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@urql/solid Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@stefanmaric stefanmaric marked this pull request as ready for review July 10, 2024 08:16
@XiNiHa
Copy link
Contributor

XiNiHa commented Sep 12, 2024

Are there any blockers for reviewing and merging the PR? I was also interested in creating the Solid integration and found this PR waiting reviews for months 😢 I'm open to both code contributing and experimenting in a real project (I guess it'll be more convenient to try this if we have pkg.pr.new set up?)

@JoviDeCroock
Copy link
Collaborator

The main issue with this PR is what is described, it requires so many changes to our workspace setup. I'll try to free up time to look at alternatives/... but it comes down to the same thing as when I initially tried to get solid setup, the testing story in solid seems lacking.

@XiNiHa
Copy link
Contributor

XiNiHa commented Sep 12, 2024

Since it looks like it's the JSX transform that causes the issue, how about using tagged template literals or HyperScript API instead of JSX? They exist for supporting no-build scenarios and I think it matches pretty well with what we want 🤔

@JoviDeCroock
Copy link
Collaborator

@XiNiHa do you want to pick that up? Reviving #3327 with either of those two solutions

@XiNiHa
Copy link
Contributor

XiNiHa commented Sep 12, 2024

Sure! I'll make a PR shortly 😋

@XiNiHa
Copy link
Contributor

XiNiHa commented Sep 13, 2024

Well I got blocked by some issues:

  • Both tagged template literals and HyperScript wrappers are broken 🤦‍♂️ so it'll be the only way to put precompiled code directly in the test code if we really want to have no-build
  • Solid Testing Library requires JSDOM environments to run, and this breaks one of the current persistent exchange tests in the repo
    • Using JSDOM everywhere will also harm performance and make SSR tests unreliable
  • Solid requires resolve.conditions in the Vite config to be set as ["development", "browser"] at least and this breaks Preact Testing Library 😵‍💫

After going through these, I just felt that it's natural to have different test environments for each package, as they could run on entirely different environments. Imagine supporting React Server components and bringing all the bundling stuffs; maybe breaking up the config would be the only option there. So I'd like to suggest to go with what we have in this PR, and make it as an opportunity to microtune each test environment per package to reflect the actual environment the package would run. Thoughts?

@JoviDeCroock
Copy link
Collaborator

@XiNiHa I'll merge this branch as is and we can look at vitest workspaces as an optimisation to reduce the configs

@JoviDeCroock JoviDeCroock merged commit a98f25c into urql-graphql:main Sep 13, 2024
13 checks passed
@JoviDeCroock JoviDeCroock mentioned this pull request Sep 13, 2024
@github-actions github-actions bot mentioned this pull request Sep 13, 2024
This pull request was closed.
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.

4 participants