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

Docs: Add guide for incremental adoption #9215

Merged
merged 20 commits into from
Mar 23, 2024

Conversation

Baltazore
Copy link
Collaborator

@Baltazore Baltazore commented Jan 29, 2024

Description

Working on guides for incremental adoption of new APIs of EmberData

Rendered

Notes for the release

guide for incremental adoption

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking good, left some small cleanups

@fry69
Copy link

fry69 commented Feb 19, 2024

  • ember-data/store (without the @) does not export CacheHandler, only @ember-data/store does, so the import { CacheHandler } line in the RequestManager service should be changed
  • import { setBuildURLConfig } seems to be unused and get marked as such by the linter

@fry69
Copy link

fry69 commented Feb 19, 2024

I also do not know how much hand-holding is needed here, but maybe it should be mentioned that the RequestManager service must be in the file app/services/request-manager.js or the @service declaration in the Store service won't find it without explicitly referring to it (e.g. @service('requests') for app/service/requests.js).

@Baltazore
Copy link
Collaborator Author

hey @fry69 thanks for review and testing, good inputs - I will update accordingly
Also please keep in mind there code here not works just yet, some fixes were made, but not yet released, especially around service injection area.

@fry69
Copy link

fry69 commented Feb 19, 2024

The goal is to get this working, right? IMHO nothing is more frustrating that simple sample code which does not work. I just want to point out some pitfalls, here are some others:

  • @ember-data/json-api in the version 4.12.x does not seem to have a query() in the /request -> Could not find module @ember-data/json-api/request imported from (require)
  • @ember-data/request-utils does not exist for 4.12.x
  • host: config.api.host inside the JSON:API query() (using EmberData 5.3) does not set the host for said query, it still talks to localhost (my old style adapter based queries work fine with the same config)
  • host: config.api.host seems to get respected if I put it into a call to setBuildURLConfig() inside the constructor() of /app/app.js
  • console.log() from TestHandler never show up even if my request now flows through this new service (and fails because of a missing API key, so it is clearly not using the old, working flow/store)?
  • even if I completely clear the this.use() array in the RequestManager, my request still comes through (and fails because of missing said API key), which leads me to believe the store service is not using the new RequestManager at all, but a different one, the @service does not seem to be enough in this case?
  • it seems to work with 5.3.0 and current alpha.22 preview release IF the code from RequestManager gets moved into the store service, otherwise I get RequestManager.useCache(<handler>) May only be invoked once errors

This the store.js that works for me:

// eslint-disable-next-line ember/use-ember-data-rfc-395-imports
import Store from 'ember-data/store';
import { CacheHandler } from '@ember-data/store';
import RequestManager from '@ember-data/request';
import Fetch from '@ember-data/request/fetch';
import { LegacyNetworkHandler } from '@ember-data/legacy-compat';

const APIKeyHandler = {
  request({ request }, next) {
    console.log('APIKeyHandler() called');
    const headers = new Headers(request.headers);
    headers.append(
      // see AuthHandler example
    );

    return next(Object.assign({}, request, { headers }));
  }
}

export default class MyStore extends Store {
  requestManager: RequestManager;

  constructor(args) {
    super(args);
    this.requestManager = new RequestManager;
    this.requestManager.use([LegacyNetworkHandler, APIKeyHandler, Fetch]);
    this.requestManager.useCache(CacheHandler);
  }
}

@Baltazore Baltazore added the 🏷️ doc This PR adds/improves/or fixes documentation label Feb 20, 2024
@Baltazore Baltazore force-pushed the incremental-migration-guide branch from 3422008 to e5b7381 Compare February 26, 2024 16:45
@fry69
Copy link

fry69 commented Feb 29, 2024

Latest version looks great, thank you @Baltazore

@Baltazore
Copy link
Collaborator Author

@runspired where do we put it? I need to update the location of file, and then fix nav links. But overall I think this is ready

@Baltazore Baltazore force-pushed the incremental-migration-guide branch 2 times, most recently from 9aed9f2 to cbb740c Compare March 2, 2024 09:47
import type Book from '../models/book';
import type Genre from '../models/genre';

setBuildURLConfig({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above this is recommended to call this function in app/app.js, for consistency, maybe it would be better to use the recommandation ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, up to change still

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app.js makes the most sense

@Baltazore
Copy link
Collaborator Author

@sly7-7 thanks for review, would need to finalize some questions

tests/incremental-json-api/app/services/store.js Outdated Show resolved Hide resolved
tests/incremental-json-api/app/services/request-manager.ts Outdated Show resolved Hide resolved
tests/incremental-json-api/app/templates/application.hbs Outdated Show resolved Hide resolved
tests/incremental-json-api/app/routes/application.ts Outdated Show resolved Hide resolved
guides/drafts/incremental-adoption-guide.md Outdated Show resolved Hide resolved
guides/cookbook/incremental-adoption-guide.md Outdated Show resolved Hide resolved
guides/cookbook/incremental-adoption-guide.md Show resolved Hide resolved
import type Book from '../models/book';
import type Genre from '../models/genre';

setBuildURLConfig({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, up to change still

@Baltazore
Copy link
Collaborator Author

@runspired apparently I don't know how to use github (only gitlab on work 😄 )
I left some questions last week, but to submit those questions I should have click a button that only visible in diff view 🤦

@Baltazore Baltazore force-pushed the incremental-migration-guide branch 3 times, most recently from 78dfe7b to ed50e5f Compare March 12, 2024 20:53
@Baltazore Baltazore force-pushed the incremental-migration-guide branch from 1782214 to e9050cc Compare March 22, 2024 09:07
@sly7-7
Copy link
Contributor

sly7-7 commented Mar 22, 2024

@Baltazore Did you just do a rebase ?

@Baltazore
Copy link
Collaborator Author

Baltazore commented Mar 22, 2024

@Baltazore Did you just do a rebase ?

yes, have I lost something ?

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 22, 2024

I don't think so, actually I was speaking with @runspired and he asked me if I could make the rebase, because he wanted to merge :)

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 22, 2024

@runspired, @Baltazore has been quickier 😄 so I guess you can merge

@Baltazore Baltazore force-pushed the incremental-migration-guide branch from aff8f21 to eb6fde2 Compare March 23, 2024 09:21
@runspired runspired changed the title Draft: Add guide for incremental adoption Docs: Add guide for incremental adoption Mar 23, 2024
@runspired runspired added the 🎯 canary PR is targeting canary (default) label Mar 23, 2024
@runspired runspired merged commit 1cfb56e into emberjs:main Mar 23, 2024
28 checks passed
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
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants