brianperry → credited coby.sher → .
I didn't see a place in the docs to update this besides for the generated API docs which should be covered here. Let me know if a new section should be added or if this makes sense in the performance section let me know.
I've done some experimentation here[...]
Looks great! I would love to turn this into some sort of CLI util that eventually could be run as a step in a `create-drupal-client-app` style starter, but given the fact that the client needs to be set up first I like the idea of this being pure documentation (with examples of course) for the first iteration.
brianperry → credited coby.sher → .
As one of the maintainers on the Drupal API Client, it would be great to see this happen to give extra legitimacy to these libraries when searching for Drupal related tools on npm.
Our issue might have to do with our typedoc declarations. I'll look into that first: https://typedoc.org/guides/declaration-references/#module-source
The maintainer fixed part of the issue in a new release, however upon installing that version and trying it with our docs I still see some broken links.
I'm going to try to update my reproduction to capture these cases. Lucky for us the maintainer of that plugin has been very responsive.
coby.sher → created an issue.
https://github.com/HiDeoo/starlight-typedoc/issues/40#issuecomment-20618...
I was able to reproduce some broken link behavior with a minimal monorepo so hopefully the maintainer can fix or we can understand how we are holding it wrong :)
I see others reporting the same issue on GitHub and the maintainer is looking for a minimal reproduction. I'll work on making that available.
MR to the MR is up for review: https://git.drupalcode.org/issue/api_client-3389583/-/merge_requests/1
Hey Marcin. Yes I think that is helpful research, thank you!
I'm interested in looking into Zod too. Something like https://github.com/rsinohara/json-to-zod looks promising.
LGTM!
Going to start poking at this.
I agree that ideally we would find something that works cross platform here but I'm not sure it exists.
If we really wanted to provide something out of the box, I think we could cook something up that uses indexedDB, cache, or local/session storage APIs in the browser, and an in-memory or file based solution for server side. The code could check the environment before selecting which to save the data to.
I wonder if a plugin system would be worth adopting? Maybe for 2.0 :)
Keeping this open for now, but my current thinking is that we work on documenting real world cache examples and postpone or close this if we're satisfied. We can then let user feedback dictate if we need to change course and ship with something.
That seems reasonable.
I spent some time going through the WebContainers tutorial. Really wild what is possible just within a browser environment - installing node packages, running a server, a fully interactive terminal and live code reload.
Ooh exciting! Would there also be a Drupal install in there? Is that practical?
Also worth pondering if this is still overkill at the end of the day :)
Maybe, but I think it is still worth doing.
Starlight seems like a good option to me!
brianperry → credited coby.sher → .
coby.sher → created an issue.
brianperry → credited coby.sher → .
I'm not as familiar with cookie based on in this context. I am probably misunderstanding these docs but is the username and password required credentials? https://www.drupal.org/docs/8/core/modules/rest/using-other-authenticati... →
In other words, what do we need to provide to get the cookie back? `xCSRFToken` makes sense. I don't know if we need `value` here unless we need it to pass to Drupal. I would expect `username` and `password if I understand correctly.
Just one small nit idk if we want to change, either way looks good to me.
Just one clarifying question on the return type
I like the approach of breaking this off into a new package. Should it extend the JsonApiClient so that a user wouldn't need to create multiple clients for all of the functionality?
Do you have any thoughts on how we'd expose that error handling to users? Anything like what we did for Drupal State: https://project.pages.drupalcode.org/drupal_state/en/error-handling/ or something different?
At the moment I figured we'd just throw the error and let the user handle it via try/catch, but we could implement an onError hook. Should that be an enhancement or part of this ticket?
Pausing this ticket to work on https://www.drupal.org/project/api_client/issues/3407051#comment-15351981 → first as the error handling will be important for this implementation
I might pick this up. I'm working on https://www.drupal.org/project/api_client/issues/3376946 → and finding a few places where it would be nice to throw and error, but this breaks a lot of the current tests. So instead of gilding that one I will start on this.
Here are my thoughts on what the API should look like:
We need to be able to return an error from `fetch`. We can change the return type to a tuple like so:
async fetch(
input: RequestInfo | URL,
init?: RequestInit,
): Promise<[Response | null, Error | null]>
This has some advantages and some disadvantages: any time `fetch` is called, the result would need to be a destructured array, and we will have to check if Error is not null. This is similar to Golang's convention which can feel kind of verbose, but does enforce error checking and handling.
We could also use an object for this but that detail isn't super important to me. The important part is actually returning and not throwing the error from `fetch`, allowing us to throw or handle the error as the end user sees fit.
Thoughts?
Thanks to all of the contributors on this one: abhisekmazumdar, shrutishende, brianperry, and pratik_kamble!
I can reproduce this with a codesandbox but I see a better error message when trying this locally with the example:
Access to fetch at 'https://dev-drupal-api-client-poc.pantheonsite.io/es/jsonapi/node/recipe...' from origin 'http://localhost:5173' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled
Looks like setting a custom header breaks the CORS? This one is beyond me a bit but I think the setting a custom header itself is working and this is a config issue one way or another. I'm going to mark this as closed.
This does highlight to me that we need better error handling throughout.
Thanks Pratik and Brian!
coby.sher → created an issue.
coby.sher → created an issue.
coby.sher → created an issue. See original summary → .
brianperry → credited coby.sher → .
coby.sher → created an issue.
@pratik_kamble I dont think it should be cached in this case. There is a use case for needing to read the headers from the response in addition to the json from Drupal. I don't think we'll want to cache the response in this case but I could be convinced otherwise.
This might highlight desire for a a per-request opt-out for the cache.
Good start, left a review on the MR.
> how cache should work with api parameter. Should we add query param(by doing base64 encoding) in the cache key?
Yeah, a hash of the query string added to the rest of the key makes sense to me.
brianperry → credited coby.sher → .
coby.sher → created an issue.
I have added a page explaining the 0.x releases ready for review here:
https://www.drupal.org/docs/develop/decoupled-drupal/api-client/scope-of... →
I am adding another page for code examples, let me know if they should be consolidated.
coby.sher → created an issue.
OK Fixed the test issue then ran into some strange mismatch of esbuild versions between tsup and the esbuild-node-pollyfill-plugin. @pratik_kamble helped me resolve it – Still unsure what the underlying cause was but to fix it I copied over a pnpm lockfile from a more recent branch and went from there.
This branch should now be up-to-date with the latest canary and ready for another round of review.
After rebasing this branch with the latest canary, the tests for the cache feature are failing. Looks like the Spies are no longer being called and I'm not really sure why. They were working previously. I will continue trying to resolve this but if anybody has any ideas or better ways to test this functionality I'm all ears :)
coby.sher → created an issue.
I have an implementation of this mostly done. I tried using Zustand for the example but there's no simple way to maintain the store between page refreshes, so for the current example I used localStorage. We could use indexedDB instead, but either way I like the idea of using the browser for the simple vite example.
It would still be helpful to show an example with Zustand or something similar in an SSR context but it would involve adding an SSR example app to the examples.
Is that out of scope here or is it worth adding a simple vite+react SSR app to the examples?
Digging into this more. Next.js for Drupal uses node-cache as their default, will probably use that. Open to suggestions if anyone has a strong opinion. A Map() might work but there are probably some lightweight options out there worth using instead.
coby.sher → created an issue.
... more complete tests and possibly improved documentation.
I am going to start working on this :)
Small implementation detail: we should consider a Map() instead of a plain Object. I have not used these in the past myself but I've seem them recently and they have a lot of benefits to a plain object including get and set methods and being iterable by default.
Objects are allowed as keys which could possibly help with the locale issue by saving the locale + the key in a object as the key.