alpheios-project / alpheios-core

Alpheios Core Javascript Packages and Libraries
15 stars 2 forks source link

Dev wordlist refactoring #538

Closed kirlat closed 4 years ago

kirlat commented 4 years ago

This is a first phase of the lexical query refactoring. It was tested for Latin and Greek mostly, and is not production ready by any means. It needs many improvements before we'll be able to use it fully. However, it shows all aspects of a redesigned architecture.

The purpose of the PR is to show the concept and offer if for discussion.

All the new files are placed into two directories: app and data-model. The former contain the files that would be used on the client side, by the app controller and other objects that belong to it. The latter contains the files responsible for data retrieval. They can theoretically be hosted on the server side (but that would require some code changes). These directory division is not final and may change in the future.

Many decisions in the code are dictated by separation of concerns. I've tried to add comments to explain design decisions, but I know it needs much more. I will add it in the next few days.

The way some code flows are implemented is temporary. It will be improved in the future.

The tests are almost non-existent, I will work on them in the next few days. I (and other people) has problems trying to make the fetch that is used by Apollo work in Jest environment. I was not able to solve it so far but hopefully I will. It, unfortunately, blocks testing of all objects that are Apollo-dependent.

Please let me know what do you think. Sorry for too many changes in one PR, but some things were impossible without changing the other and so on. One thing leads to another.

kirlat commented 4 years ago

from https://github.com/alpheios-project/alpheios-core/pull/538#discussion_r496672962

I think it would help to know when we expect this method to be used. In what scenarios would we want the default callback?

@balmas This method is not used in the current implementation. Should I remove it according to the YAGNI principle?

balmas commented 4 years ago

from #538 (comment)

I think it would help to know when we expect this method to be used. In what scenarios would we want the default callback?

@balmas This method is not used in the current implementation. Should I remove it according to the YAGNI principle?

I guess it depends upon whether you expect us to need it. If you don't have a clear idea of when we might use it then I would vote for removing it.

kirlat commented 4 years ago

As I understand it, the resolver of a query should be behind the graphql API. But it looks to me like we use this resolver as the read function for the InMemoryCache. And it's not clear to me where the query execution actually happens

We specify an endpoint of "/graphql" when we create the ApolloClient but that doesn't correspond to any real endpoint. So it looks like it's handing it off to the cache to do the actual resolution of the query.

That is correct, the "/graphql" endpoint is just a convention and is not really used. The query is fully local because all its fields (i.e. a slinge word field for now) has an @client (that's an indication that a resolution for this field should happen locally):

query WordQuery($langCode: Language, $userID: String) {
      word(langCode: $langCode, userID: $userID) @client
    }

So this query is resolved from the Apollo's in-memory cache.

I would expect the cache to have a reference to an adapter method to convert the JSON of the results of the query resolution (or execution) into the HomonymGroup object, but instead it seems to responsible for actually resolving the query, which seems a little backwards.

This is how I've seen it when was designing an architecture. WordQueryAdapter is the last object on the client side, right in front of a separation line between the client and the server. It is responsible for communicating with the "server". It's purpose is to hide implementation details of the query from the client. WordQueryAdapter holds the Apollo in-memory cache that belongs to the client side: https://www.apollographql.com/docs/react/local-state/local-state-management/.

However, if we want to consider the possibility of the word query to be remote, the resolution logic should belong to the server side part of business logic. This is the WordQueryResolver is. It is responsible for resolving the incoming GraphQL query and it uses a WordQuery class for that. WordQueryAdapter imports the WordQueryResolver (because there is no other way to implement a local field resolution). I see this connection as a bridge between "local" and the "remote" code.

Does this make sense? What do you think?

balmas commented 4 years ago

As I understand it, the resolver of a query should be behind the graphql API. But it looks to me like we use this resolver as the read function for the InMemoryCache. And it's not clear to me where the query execution actually happens We specify an endpoint of "/graphql" when we create the ApolloClient but that doesn't correspond to any real endpoint. So it looks like it's handing it off to the cache to do the actual resolution of the query.

That is correct, the "/graphql" endpoint is just a convention and is not really used. The query is fully local because all its fields (i.e. a slinge word field for now) has an @client (that's an indication that a resolution for this field should happen locally):

query WordQuery($langCode: Language, $userID: String) {
      word(langCode: $langCode, userID: $userID) @client
    }

So this query is resolved from the Apollo's in-memory cache.

I would expect the cache to have a reference to an adapter method to convert the JSON of the results of the query resolution (or execution) into the HomonymGroup object, but instead it seems to responsible for actually resolving the query, which seems a little backwards.

This is how I've seen it when was designing an architecture. WordQueryAdapter is the last object on the client side, right in front of a separation line between the client and the server. It is responsible for communicating with the "server". It's purpose is to hide implementation details of the query from the client. WordQueryAdapter holds the Apollo in-memory cache that belongs to the client side: https://www.apollographql.com/docs/react/local-state/local-state-management/.

However, if we want to consider the possibility of the word query to be remote, the resolution logic should belong to the server side part of business logic. This is the WordQueryResolver is. It is responsible for resolving the incoming GraphQL query and it uses a WordQuery class for that. WordQueryAdapter imports the WordQueryResolver (because there is no other way to implement a local field resolution). I see this connection as a bridge between "local" and the "remote" code.

Does this make sense? What do you think?

Yes this makes much more sense to me now. Thank you for the link to the relevant Apollo Client docs. I think your approach is a good start. So is this correct:

In this architecture, we are also still using the Client Adapters to execute domain business logic to turn individual service results into domain objects. I think this double usage of the term "Adapter" is adding some confusion for me here. I don't think the WordQueryAdapter is actually performing an adapter role. Would "WordQueryController" or "WordQueryContainer" be more accurate?

kirlat commented 4 years ago

Would "WordQueryController" or "WordQueryContainer" be more accurate?

I think you're right that Adapter is not the appropriate term to describe the WordQueryAdapter responsibilities. I think we can rename it to the WordQueryController, if you don't mind. It will fit well into the "Controller" role within the DDD terminology.

kirlat commented 4 years ago

I will close this PR in favor of a new one, in order to keep discussion more manageable