urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.6k stars 448 forks source link

Add solid #3327

Open Nvos opened 1 year ago

Nvos commented 1 year ago

Resolves #3303

Summary

Add new urql-solid package implementing solid-js bindings.

Api is nearly same as for react/preact with difference being that some props are passed as union of value and accessor.

Set of changes

Adds following solid-js hooks

Nvos commented 1 year ago

Seems that CI is failing on typecheck in project root, given that solid requires different jsx and jsxImportSource than react typecheck with current configuration will fail as solid package extends base tsconfig and base tsconfig includes all packages thus runs typecheck with jsx set to react and without jsxImportSource.

It might be good idea to run each sub package commands instead e.g. pnpm run -r {cmd} or use monorepo tools (turbo/nx) to run commands in sub packages.

kitten commented 1 year ago

This is only a preliminary review, but I'll get back to the details once we get closer to having a final implementation. I'm fine with fixing up the Vitest and TypeScript configs to be inline with what we'd expect in this repo, but I'll get back to that closer to having it done as a last step ✌️

Nvos commented 1 year ago

Thanks, will work on those comments this weekend

Nvos commented 11 months ago

Is there anything else from my side to do to push this PR forward?

kitten commented 11 months ago

@Nvos: Hiya 👋 Sorry for the delay. Mainly, what I got stuck on here is the setup. While I haven't normalised, rebased and checked everything for discrepancies yet, I started with aligning some of the setup changes.

Firstly, the tsconfig changes are pretty trivial and we can even avoid JSX to not have to deal with it that often. React is a little special in that regards and Typescript doesn't have many great options to isolate builds without becoming the “sole/primary build tool”

Anyway, from a Vitest standpoint, the config for it for Solid should obviously not be a snowflake and unique. Having it all run as one thing (besides some of the E2E tests which are mostly to prevent common regressions instead; added as an experiment) would be neat.

That said, the tests are failing now and I don't have too much time right now to debug this myself (Sorry!) One thing I discovered is that there's several waitFor checks that aren't await-ed properly. I didn't get rid of these in any test besides createQuery's.

When I'm looking at the tests triggering effects, the primary thing I'm noticing is that Solid's test framework lacks an act function or another mechanism of flushing effects. Even waiting for a random timeout or micro-tick doesn't seem to really convince it to consistently execute effects in a predictable manner that I'd expect from tests. It's a little puzzling.

On a side-note, we're missing a small change that tests that the createSubscription scan function actually executes. But that's a small detail. Overall, I haven't tried this in a Vite sandbox, and set up an example yet, but from the tests, my confidence right now in them isn't particularly high until I can understand how Solid expects effects to flush in tests 😅

Here's a commit with the relevant changes: https://github.com/urql-graphql/urql/commit/584d58d36271725c0e8a25df6796d777a3ce8bb1

Nvos commented 11 months ago

I aggree that having 1 vitest config is good, thought thought I still would like to have solid plugin in vitest instead of using solid-js/h maybe it would be good idea to merge main config if some changes are required:

// other imports...
import rootConfig from '../../vitest.config';

export default mergeConfig(
  rootConfig,
  defineProject({
    plugins: [solidPlugin()],
    test: {
      globals: true,
    },
  })
);

Though plugin is required only to handle JSX, we could remove those tests (those are only required to test suspense) and instead introduce some E2E tests via playwright/cypress. I could write playwright tests, as for cypres would require to do some research as didn't use it for long time now.

I'm not that proficient in writing unit tests in solid. Tests in this PR were based on my initial research. Though I noticed that there's test helper testEffect which might be what we need. I'll take a look at what we can do in regard to handling effects in tests this week.

In regard to:

That said, the tests are failing now and I don't have too much time right now to debug this myself (Sorry!)

Do you mean CI or tests were failing when running locally?

changeset-bot[bot] commented 11 months ago

⚠️ No Changeset found

Latest commit: 9d6836e0e9f586dd18570eedd0558f9412a81178

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Nvos commented 11 months ago

Updated all tests to use testEffect, it should be more reliable now.

Tried few things and as for now I'm not sure how to handle running test and check from repository root without larger changes as those do not respect configurations in nested packages.

Tried to change to use solid/h as you did on your branch but for some reason reactivity was not working inside components. Will take a look at it this weekend as it seems to be only way to use vitest.config from repository root

Nvos commented 11 months ago

After some research I come to conclusion that with current setup it seems unlikely to run solid tests from project root via test script due to vite-plugin-solid seemingly being necessary to run hook tests (seems that createResource reads sharedConfig.context.id and context is never set without plugin). I have tried to handle this in 2 ways:

From my perspective best way to handle such cases globally would be to switch from running tests via single config with includes to respective configs in sub-packages which could be ran by pnpm or preferably turbo. There could be single base config which could be used as default if there's no changes needed.

Another way could be handling solid-urql separately by excluding it from current config and running it along side current tests but this makes solid-urql special case which might not be preferable

stefanmaric commented 3 months ago

Fixing the TypeScript errors was fairly easy: https://github.com/urql-graphql/urql/pull/3607/commits/398267e70e42dced7867627795b916eb6c4bbe98

On the other hand, I spent quite some time trying to get tests running with Vitest's Workspaces, but to no avail. I tried different combinations of inline vs. standalone project configs, and not even with the v2 beta which forks by default, was I able to get it to work.

This thread might be relevant for this issue: https://github.com/vitest-dev/vitest/issues/5301.

I rebased this branch and got it to kinda work with pnpm recursive: https://github.com/urql-graphql/urql/actions/runs/9435968855/job/25990068426.

Not without some downsides: in CI, it will run tests for each package individually (but in parallel), while locally pnpm run test will run the tests from the top level in watch mode as it has been doing so far, but it will skip solid-urql's tests.

So, if your changes break the Solid integration, you will only notice after pushing. Not ideal, but it might be worth it.

Solid + urql is a magic combo I would love to have. ❤️