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

WIP: Serialization handling for cacheability #44

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

olivermack
Copy link

This is for #43

This adds an ApiSerializer which converts to a json-serializable POJO in both directions. Please let me know if this approach conflicts with any best practices or design considerations of this lib.

Best

@olivermack olivermack changed the title API: Serialization handling for cacheability WIP: Serialization handling for cacheability Jan 17, 2019
@olivermack
Copy link
Author

Can anyone help me to get the idea here? I'm not that much into flow and I'm just curious about the linter errors: obviously the linter requires to specify the readable properties in the class definition - but most of the classes here do not really have those property definitions; instead, the properties are defined on runtime via Object.defineProperty() ... that basically means that the properties in fact may - or may not - exist, depending on the runtime circumstances and thus they cannot be checked statically...

In the serializer of this PR I make use of the three dots for rest and spread... Basically for one reason: I only want to extract those parameters from the objects where the serializer needs to perform special actions - by this I was hoping to keep the stuff as flexible as possible when in comes to changes of the properties of the objects.

Any hints? I mean, why is then using flow at the end when the values are set magically in the object's constructors anyway? Why not setting those typed options containers as an member as is?

type ApiOptions = {
  title?: string,
  // ...
};

export default class Api {
  entrypoint: string;
  options: ApiOptions;

  constructor(entrypoint: string, options: ApiOptions = {}) {
    this.entrypoint = entrypoint;
    this.options = options;
  }
}

README.md Outdated Show resolved Hide resolved
src/ApiSerializer.js Outdated Show resolved Hide resolved
@dunglas
Copy link
Member

dunglas commented Jan 28, 2019

Can you also check why the tests are failing please? It looks related to this change.

@dunglas dunglas requested a review from mauchede January 28, 2019 09:23
@olivermack
Copy link
Author

@dunglas thanks for checking. It's actually not the tests themselves that fail (as far as I can assess) it's the linter that struggles because I'm spreading member variables from the api-objects which are dynamically declared... this is actually what my last comment was about.

@olivermack
Copy link
Author

Hey... any news? I still need to know how to proceed here.

@dunglas
Copy link
Member

dunglas commented Feb 18, 2019

Sorry for the lack of response. We’ll try to review ASAP. Having Travis green is mandatory.

@olivermack
Copy link
Author

Having Travis green is mandatory

Yah, sure, no question... This is what my initial question was about which I cannot solve on my own...

Base automatically changed from master to main January 24, 2021 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants