[WIP] TypeScript #175

buschtoens · 2018-10-14T13:18:28Z

Closes #140.

I had some spare time and wanted to start converting this addon over to TypeScript.
Unfortunately it doesn't fully work yet. But I'd like to gather some feedback already. 😊

addon/services/apollo.ts

buschtoens · 2018-10-14T13:20:45Z

addon/services/apollo.ts

-  apiURL: alias('options.apiURL'),
-  requestCredentials: alias('options.requestCredentials'),
+export default class ApolloService extends Service {
+  client!: ApolloClient<any>;


I don't like using any here, but I'm not smart enough to know a more appropriate solution.

buschtoens · 2018-10-14T13:22:50Z

addon/apollo/query-manager.ts

-    this.set('activeSubscriptions', A([]));
-  },
+export default class QueryManager extends EmberObject {
+  @service apollo!: ApolloService;


Is the QueryManager guaranteed to only be instantiated through ApolloService#create QueryManager? In that case we should remove the @service injection here and make apollo a constructor parameter.

We can likely also drop the extends EmberObject bit.

buschtoens · 2018-10-14T13:25:18Z

addon/index.ts

+export function getObservable(queryResult: {
+  [apolloObservableKey]: ObservableQuery;
+}): ObservableQuery {
+  return queryResult[apolloObservableKey];


I reckon that this is actually what was intended here.

In the original index.js this was done:

https://github.com/bgentry/ember-apollo-client/blob/98c5880830358d0506f5cc95ac23469ff69be7f7/addon/index.js#L5-L9

josemarluedke · 2018-11-01T23:11:26Z

@buschtoens Thanks for starting the work on this. I'm very exited to have this addon in TS.

@bgentry I think it would be nice to start a new branch where we can merge the TS work into. That would allow us to move piece by piece, and so, once we are happy with the initial TS refactor, we could cut a beta release.

buschtoens · 2018-11-02T14:28:35Z

ember-cli-typescript is available as an alpha for doing the transpilation with Babel. I can upgrade this PR to do that, however I'll then need to replace ember-auto-import with a custom setup, because of the aforementioned incompatibility.

dfreeman · 2018-11-06T15:42:01Z

It might be worthwhile to look at adding Babel 7 support to ember-auto-import rather than building custom import stuff here.

@ef4 said here that he'd ultimately like to have auto-import use a Babel plugin for discovering imports, which would avoid having to double-parse everything. That makes a ton of sense as a goal, but will also require some careful caching work to get right.

Maybe separating the Babel 7 support from that work and doing a first pass that continues with the status quo but can work with either the Babel 6 or 7 parser would be a good first step?

josemarluedke · 2018-11-26T20:46:19Z

🎉 ember-auto-import v1.2.16 has now support for Babel 7. We should be unblocked now to move this PR further.

buschtoens · 2018-11-27T08:07:50Z

Awesome! Maybe I can chip in some further work this evening. :)

bgentry · 2018-12-05T19:58:59Z

@buschtoens Thank you for updating this! Is this ready for a review now that it's using the typescript-capable ember-auto-import?

This would actually be my first real experience with TypeScript. Is there anybody with more experience around it that can give this PR a review?

buschtoens · 2018-12-05T20:07:48Z

From the technical setup POV this is ready for a review. Only thing missing is failing the build for type errors, just because there are so many of them right now.

For all of this to make sense as a whole we also need #139, which can already be shipped before this PR.

The actual source code itself also needs some further TLC. Before merging this, I would like to remove all type errors. But until then we can already start reviewing this! :)

buschtoens · 2018-12-06T10:29:40Z

I've fixed all type errors. The remaining three errors are caused by missing *.d.ts for the test fixture *.gaphql:

tests/unit/services/apollo-test.ts:6:26 - error TS2307: Cannot find module '../build/test-mutation'.

6 import testMutation from '../build/test-mutation';
                           ~~~~~~~~~~~~~~~~~~~~~~~~

tests/unit/services/apollo-test.ts:5:23 - error TS2307: Cannot find module '../build/test-query'.

5 import testQuery from '../build/test-query';
                        ~~~~~~~~~~~~~~~~~~~~~

tests/unit/services/apollo-test.ts:6:26 - error TS2307: Cannot find module '../build/test-mutation'.

6 import testMutation from '../build/test-mutation';
                           ~~~~~~~~~~~~~~~~~~~~~~~~

In order to be able to enforce type checking and being able to merge this, we could codegen them statically for now.

I have no hands-on experience with GraphQL / Apollo (or this addon even 😂) yet, so I don't know what the preferred way for doing codegen is. Are these *.d.ts checked into the repo? Do we as ember-apollo-client need to provide automatic codegen to the consumer or is this something an IDE does?

What's the workflow here?

buschtoens · 2018-12-06T10:33:18Z

Dang, Ember 2.12 is failing. Considering that the LTS schedule for 2.12 has expired, can we just remove it, if fixing this is non-trivial?

josemarluedke · 2018-12-06T16:13:50Z

@buschtoens For the graphql files, we have a few options to proceed:

Add @ts-ignore to these lines
Add type definitions for these
Allow importing files with .graphql extension (issue in broccoli-graphql-filter)

Here is what a fine definition would look like for the .graphql files

declare module '*.graphql' {
  import { DocumentNode } from 'graphql';
  const defaultDocument: DocumentNode;

  export default defaultDocument;
}

For codegen, I have been using https://graphql-code-generator.com/ for that and works great.

addon/services/apollo.ts

bgentry · 2018-12-06T17:46:49Z

Dang, Ember 2.12 is failing. Considering that the LTS schedule for 2.12 has expired, can we just remove it, if fixing this is non-trivial?

Yes, I am absolutely down with removing old releases that are outside of LTS. I haven't done an ember-cli-update in awhile but that would probably include such a change.

dfreeman · 2018-12-06T19:19:58Z

I probably won't get to it in the next day or two, but I'm happy to give this a review next week if it's helpful.

bgentry · 2018-12-17T18:56:05Z

This needs a rebase now that the Ember upgrade has been merged (thanks for that @buschtoens!).

@dfreeman have you or anybody else w/ TypeScript experience gotten to review this?

josemarluedke · 2019-01-11T19:36:21Z

I'm seeing the same problem with the duplicated graphql error. I have tried to add graphql as a peer dep, and it doesn't seem to solve it. @buschtoens If you could also take a look on that, it would be awesome. I will keep researching it.

josemarluedke · 2019-01-11T19:36:56Z

I have submitted a PR to rebase this branch with latest changes on master:

buschtoens#8

buschtoens · 2019-01-11T20:59:26Z

I'll have a look (latest) tomorrow, maybe already this evening. Thanks already!

buschtoens · 2019-01-12T18:11:51Z

My analysis in #175 (comment) of the graphql duplication bug was not 💯correct, but was thoroughly explained in embroider-build/ember-auto-import#170. In there Ed has explained that graphql needs to be defined as a peerDependency of ember-apollo-client and then as an actual dependency of the host app, similar to how it is done in graphql-tools. @josemarluedke has applied the fix in c0f1a0e.

However, Ed still uncovered a bug that is preventing our tests from going green: embroider-build/ember-auto-import#175

josemarluedke · 2019-01-12T18:30:57Z

addon/services/apollo.ts

@@ -97,27 +112,30 @@ export default Service.extend({
   * @return {!Object}
   * @public
   */
-  clientOptions: computed(function() {
+  @computed


Is there a need to make this a computed even if they are not dependent of any properties? Or should we add link,cache here?

Suggested change

@computed

@computed('link', 'cache')

It is computed to be cached.

josemarluedke · 2019-01-12T18:31:23Z

addon/services/apollo.ts


-  cache: computed(function() {
+  @computed


This computed probably can be removed.

josemarluedke · 2019-01-12T18:32:11Z

addon/services/apollo.ts

-    let uri = this.get('apiURL');
-    let requestCredentials = this.get('requestCredentials');
-    const linkOptions = { uri, fetch };
+  @computed


Same here, should we add dependent keys here?

Suggested change

@computed

@computed('requestCredentials', 'apiURL')

If I understand the original intention correctly, these three fields are computed without dependent keys, so that they are only computed once and are immutable afterwards, which makes sense because you cannot change these options, after the Apollo client has been instantiated.

See #186.

josemarluedke · 2019-01-12T18:33:15Z

index.js

@@ -29,7 +29,7 @@ module.exports = {
        toTree(tree) {
          const GraphQLFilter = require('broccoli-graphql-filter');

-          return new GraphQLFilter(tree);
+          return new GraphQLFilter(tree, { keepExtension: true });


To keep backward compatibility, we need to add this as a configuration.

josemarluedke · 2019-01-12T18:34:15Z

tests/dummy/app/services/apollo.ts

-export default ApolloService.extend({
-  cache: computed(function() {
+export default class extends ApolloService {
+  @computed


Probably doesn't need to be a computed.

Same here. It's a computed to make it cache / lazily instantiated. Using an assignment would have the same effect, but is eagerly evaluated. Just removing the @computed and making this a regular getter would return a new InMemoryChache instance every time.

josemarluedke · 2019-01-12T18:36:26Z

tests/unit/services/apollo-test.ts

+      // Override the clientOptions.
+      @computed
+      get clientOptions() {
+        const { get } = getSuperComputed(this, 'clientOptions');


This is interesting. I wonder if this needs to be a computed property at all, if it doesn't then we could use use super.clientOptions.

I agree that these acrobatics are not really ideal. But I understand why it was implemented this way (see above comments). Maybe we can still find an alternative to computed.

ember-graphql#175 (comment)

buschtoens · 2019-01-26T18:53:41Z

Regarding the type generation for .gql, we might want to wait for typed-ember/ember-cli-typescript#192 instead?

Edit: lol, the RFC even mentions ember-apollo-client. Should have kept reading first. 😆

bgentry · 2019-03-09T19:10:07Z

@buschtoens @josemarluedke are either of you able to summarize the current status of this PR? I'd really like to get it merged, but not actually sure what the remaining issues are. We're still on the 2.x beta series so it would be a good time to get any required breaking changes merged in.

I also merged subscription support so there might be slightly more surface area to convert.

josemarluedke · 2019-03-09T19:27:57Z

@bgentry I'm really looking forward to move this forward, but we are blocked by this issue in ember-auto-import.

I'm wondering how can we keep moving the codebase for better TS support, a few things come to mind:

Move away from mixins
Convert to native classes
Decorators

I will start working on some of the above. I'm also looking forward to use this addon in Glimmer components (within ember).

bgentry · 2019-03-09T20:03:10Z

If you think that ember-auto-import issue will be fixed soon, definitely ok to wait for that. Otherwise I like the approach you outlined. Don’t need to keep blocking on another issue if we can make obvious incremental improvements sooner.

josemarluedke · 2019-08-01T16:39:28Z

As this PR stands at the moment, it would be very hard to land as it is. I will close this for now and we will use this as a reference for the TypeScript implementation. We still want to move to TS and with the recent refactorings, this will be easier now.

Thanks @buschtoens for your hard work here.

buschtoens · 2019-08-01T16:42:10Z

Awesome 🚀

buschtoens commented Oct 14, 2018

View reviewed changes

addon/services/apollo.ts Show resolved Hide resolved

buschtoens commented Oct 14, 2018

View reviewed changes

buschtoens mentioned this pull request Nov 6, 2018

Compatible with babel 7 embroider-build/ember-auto-import#119

Closed

buschtoens force-pushed the refactor/140-typescript branch from 11ab836 to c6d757c Compare December 5, 2018 19:12

josemarluedke reviewed Dec 6, 2018

View reviewed changes

addon/services/apollo.ts Show resolved Hide resolved

buschtoens mentioned this pull request Dec 17, 2018

chore: upgrade ember-cli & dependencies #178

Merged

buschtoens force-pushed the refactor/140-typescript branch from 47a8132 to ca88c0c Compare December 17, 2018 19:53

buschtoens and others added 8 commits December 17, 2018 22:21

chore(deps): add ember-cli-typescript

83b3047

chore(deps): add @ember-decorators/service

a8095fa

chore(deps): add @ember-decorators/object

46554fb

chore(tsconfig): enable experimentalDecorators

777a5fc

chore(tsconfig): make fetch import work

0e75983

refactor: first stab at a TS conversion

d891ca7

fix(deps): upgrade to latest and reatest typescript

53e9a27

fix: some new semantics

e44e5f8

buschtoens and others added 7 commits January 11, 2019 11:11

test(services/apollo): fix broken clientOptions override test

0b19eb1

chore: fix code-based type errors

b04da24

fix(services/apollo): pass args for init

2af17cc

chore(deps): upgrade to the latest and greatest

c784091

refactor: PoC for .graphql imports

86a7b78

Update yarn.lock after rebase

dcd631f

Merge branch 'refactor/140-typescript' into refactor/140-typescript

9cfa421

Move graphql to peerDependencies

c0f1a0e

buschtoens force-pushed the refactor/140-typescript branch from 12f8a02 to c0f1a0e Compare January 12, 2019 17:59

josemarluedke reviewed Jan 12, 2019

View reviewed changes

refactor(types): remove rest params in framework hooks for TS

4140e12

ember-graphql#175 (comment)

josemarluedke mentioned this pull request Feb 22, 2019

Common chunks can end up in both vendor.js and test-support.js embroider-build/ember-auto-import#175

Open

josemarluedke mentioned this pull request Mar 9, 2019

Move graphql to peerDependencies #260

Merged

josemarluedke added the Status: Abandoned label Aug 1, 2019

josemarluedke closed this Aug 1, 2019

[WIP] TypeScript #175

[WIP] TypeScript #175

Conversation

buschtoens commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemarluedke commented Nov 1, 2018

buschtoens commented Nov 2, 2018

dfreeman commented Nov 6, 2018

josemarluedke commented Nov 26, 2018 • edited Loading

buschtoens commented Nov 27, 2018

bgentry commented Dec 5, 2018

buschtoens commented Dec 5, 2018 • edited Loading

buschtoens commented Dec 6, 2018 • edited Loading

buschtoens commented Dec 6, 2018

josemarluedke commented Dec 6, 2018

bgentry commented Dec 6, 2018

dfreeman commented Dec 6, 2018

bgentry commented Dec 17, 2018

josemarluedke commented Jan 11, 2019

josemarluedke commented Jan 11, 2019

buschtoens commented Jan 11, 2019

buschtoens commented Jan 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buschtoens commented Jan 26, 2019 • edited Loading

bgentry commented Mar 9, 2019

josemarluedke commented Mar 9, 2019

bgentry commented Mar 9, 2019

josemarluedke commented Aug 1, 2019

buschtoens commented Aug 1, 2019

josemarluedke commented Nov 26, 2018 •

edited

Loading

buschtoens commented Dec 5, 2018 •

edited

Loading

buschtoens commented Dec 6, 2018 •

edited

Loading

buschtoens commented Jan 26, 2019 •

edited

Loading