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

Include param should always reference array in docs #9507

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Baltazore
Copy link
Collaborator

@Baltazore Baltazore commented Jul 9, 2024

Due to confusion described in #9502 we should update docs and maybe internal usage

TODO:

@Baltazore Baltazore added 🎯 canary PR is targeting canary (default) 🏷️ doc This PR adds/improves/or fixes documentation labels Jul 9, 2024
@mkszepp
Copy link
Contributor

mkszepp commented Jul 9, 2024

In you checklist we need to add one point, that we need todo, before we ship lint, updating docs... otherwise api isn't working correctly with includes, when users are using legacy code-parts.

Atm all legacy codes like this.store.findAll / this.store.findRecord are passing include not correctly to api.

When we pass a string[] to include (include: ['comments', 'author']) the urls is:
api/post?include[]=comments&include[]=author

Correctly is:
api/post?include=comments,author

This fix we should also ship to LTS versions, because the current lts are making this mistake

@runspired
Copy link
Contributor

@mkszepp as I said in the ticket, we'd need to look at your particular setup to determine why that is, as that's up to the adapter to do

@mkszepp
Copy link
Contributor

mkszepp commented Jul 10, 2024

@mkszepp as I said in the ticket, we'd need to look at your particular setup to determine why that is, as that's up to the adapter to do

its not my stetup directly... the bug is already with default json-api adapter (like already told in issue)... here the repo with incorrect api call mkszepp/ember-data-ts-errors@152a105)

grafik

Edit:
I think the problem is this code part...

buildParams(prefix + '[' + (typeof obj[i] === 'object' && obj[i] !== null ? i : '') + ']', obj[i], s);

for json-api its always correct when its not include as string[], but this code part will be used inside rest-adapter (see https://github.com/emberjs/data/blob/main/packages/adapter/src/rest.ts#L1466), so we need any override inside json-api adapter to fix this

Should we create an additional issue for this bug or do we handel this with my initial issue? Because its not directly part of this PR (only related).

I just wanted to point out in this PR that some other fixes are necessary before we merge this (as it is missing in todo list), because you can't force in docs... to use string[] when its not working with legacy adapter.

@Baltazore Baltazore force-pushed the include-should-be-an-array branch from 61f2d24 to 69923b6 Compare July 12, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎯 canary PR is targeting canary (default) 🏷️ doc This PR adds/improves/or fixes documentation
Projects
Status: needs triage
Development

Successfully merging this pull request may close these issues.

4 participants