-
-
Notifications
You must be signed in to change notification settings - Fork 452
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 solid #3327
Add solid #3327
Conversation
Seems that CI is failing on typecheck in project root, given that solid requires different It might be good idea to run each sub package commands instead e.g. |
const got = unwrap(result); | ||
if (got.fetching) { | ||
return; | ||
} | ||
|
||
resolve(got); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite feel this will be the best approach to be honest. Especially, since the inputs can change while we're waiting to resolve a promise.
I also haven't checked this in detail, but I'm very hesitant to use this approach of resolving + re-triggering this as per the refetch
call. This seems a little brittle to me.
You can see how the vue
bindings do this:
urql/packages/vue-urql/src/useQuery.ts
Lines 389 to 403 in 344b544
const promise = new Promise<UseQueryState<T, V>>(resolve => { | |
if (!source.value) return resolve(state); | |
let hasResult = false; | |
sub = pipe( | |
source.value, | |
subscribe(() => { | |
if (!state.fetching.value && !state.stale.value) { | |
if (sub) sub.unsubscribe(); | |
hasResult = true; | |
resolve(state); | |
} | |
}) | |
); | |
if (hasResult) sub.unsubscribe(); | |
}); |
The only difference is that we wouldn't necessarily want to check for stale
. That depends on how background fetches should be handled.
Maybe it does make sense though to include this, since not using suspense would be the escape hatch here. But since in these bindings there's no concept of conditionally using the resource, it's likely better not to check for stale
The important part here is that the promise actually only reacts to the result. While the source
isn't shared in the Vue implementation and instead relies on the semaphore logic in the Client
, it's also fine to try to share it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to implement it in similar way as in vue, technically it seems to be working (you can take a look at 604c6a5). Thought after working a bit on it I'm not sure if this is good idea as it results in dependency on subscription order as subscription to fetch data has to be made before subscription in createResource
fetcher.
Furthermore I'm not 100% sure if there wont be cases where order is different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order should be fine after some testing I don't think there should be any edge cases
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 ✌️ |
Thanks, will work on those comments this weekend |
Is there anything else from my side to do to push this PR forward? |
@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 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!) When I'm looking at the tests triggering effects, the primary thing I'm noticing is that Solid's test framework lacks an On a side-note, we're missing a small change that tests that the Here's a commit with the relevant changes: 584d58d |
I aggree that having 1 vitest config is good, thought thought I still would like to have solid plugin in vitest instead of using // 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 In regard to:
Do you mean CI or tests were failing when running locally? |
…t, extract query suspense tests
|
Updated all tests to use Tried few things and as for now I'm not sure how to handle running Tried to change to use |
After some research I come to conclusion that with current setup it seems unlikely to run solid tests from project root via
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 Another way could be handling |
Fixing the TypeScript errors was fairly easy: 398267e 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: vitest-dev/vitest#5301. I rebased this branch and got it to kinda work with Not without some downsides: in CI, it will run tests for each package individually (but in parallel), while locally 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. ❤️ |
Superseded by #3607 - Thank you so much for pioneering this work! |
Resolves #3303
Summary
Add new
urql-solid
package implementingsolid-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
hookscreateQuery
createMutation
createSubscription