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

Remove unneeded Js.Object.assign #1230

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@ulrikstrid
Copy link
Contributor

ulrikstrid commented Feb 23, 2018

The Object.assign here seems to be unnecessary as we don't mutate or add more fields to it. We also doesn't save it for later use so we don't really care if it's mutated in JS-land either.

@MoOx

This comment has been minimized.

Copy link
Member

MoOx commented Feb 23, 2018

@bloodyowl any comment on why did you made this in the first place?

@MoOx

This comment has been minimized.

Copy link
Member

MoOx commented Feb 23, 2018

@ulrikstrid Tests are broken so maybe there is a reason it has been made this way :) (not sure yet)

@ulrikstrid

This comment has been minimized.

Copy link
Contributor

ulrikstrid commented Feb 23, 2018

Seems to be a typing issue when returning plain objects as the switch returns different kinds of objects. I will try to work around it.

@bloodyowl

This comment has been minimized.

Copy link
Contributor

bloodyowl commented Feb 23, 2018

yeah, it was a nasty way to get the typing to match. optional keys should be marked as Js.Nullable.t('a)

@MoOx

This comment has been minimized.

Copy link
Member

MoOx commented Feb 26, 2018

@ulrikstrid do you think you can fix the all thing?

@ulrikstrid

This comment has been minimized.

Copy link
Contributor

ulrikstrid commented Feb 26, 2018

There is no solution that is as clean as I would have wanted. Maybe just add a comment?

@MoOx

This comment has been minimized.

Copy link
Member

MoOx commented Feb 26, 2018

A comment lgtm.

@ulrikstrid

This comment has been minimized.

Copy link
Contributor

ulrikstrid commented Feb 26, 2018

I can add a comment later today, or someone else can do it now.

It's pretty much the way it is because OCaml can't type multiple return types from the same function without wrapping it in another type.

@bloodyowl

This comment has been minimized.

Copy link
Contributor

bloodyowl commented Feb 26, 2018

I’ll do it tonight

@ulrikstrid ulrikstrid force-pushed the ulrikstrid:remove-unneeded-object-assign branch from 324c7a6 to 3c160c3 Feb 26, 2018

@ulrikstrid

This comment has been minimized.

Copy link
Contributor

ulrikstrid commented Feb 26, 2018

@MoOx @bloodyowl I added a comment, is it ok?

@ulrikstrid

This comment has been minimized.

Copy link
Contributor

ulrikstrid commented Feb 26, 2018

Another solution would be this:

let query = queryConfig =>
  switch queryConfig {
  | Item(queryConfigItem) =>
      {
        "path": queryConfigItem.path,
        "id": Js.Undefined.return(queryConfigItem.id),
        "by": Js.Undefined.empty,
        "value": Js.Undefined.empty,
        "order": Js.Undefined.empty,
        "limit": Js.Undefined.empty,
        "after": Js.Undefined.empty
      }
  | List(queryConfigList) =>
      {
        "path": queryConfigList.path,
        "id": Js.Undefined.empty,
        "by": Js.Undefined.from_opt(queryConfigList.by),
        "value": Js.Undefined.from_opt(queryConfigList.value),
        "order": Js.Undefined.from_opt(queryConfigList.order),
        "limit": Js.Undefined.from_opt(queryConfigList.limit),
        "after": Js.Undefined.empty
      }
  | PaginatedList(queryConfigPaginatedList) =>
      {
        "path": queryConfigPaginatedList.path,
        "id": Js.Undefined.empty,
        "by": Js.Undefined.from_opt(queryConfigPaginatedList.by),
        "value": Js.Undefined.from_opt(queryConfigPaginatedList.value),
        "order": Js.Undefined.from_opt(queryConfigPaginatedList.order),
        "limit": Js.Undefined.from_opt(queryConfigPaginatedList.limit),
        "after": Js.Undefined.from_opt(queryConfigPaginatedList.after)
      }
  };

This will generate JS like this:

        return {
                path: queryConfigItem[/* path */0],
                id: queryConfigItem[/* id */1],
                by: undefined,
                value: undefined,
                order: undefined,
                limit: undefined,
                after: undefined
              };

Would that be OK?

@MoOx

This comment has been minimized.

Copy link
Member

MoOx commented Feb 26, 2018

@ulrikstrid thanks for your input. @bloodyowl improved the situation in #1232. Are you ok with it?

@MoOx MoOx closed this Feb 26, 2018

@bloodyowl

This comment has been minimized.

Copy link
Contributor

bloodyowl commented Feb 26, 2018

we'll need to adapt the JS code a bit (as it relies on hasOwnProperty checks, but it'll be alright real soon)

@ulrikstrid

This comment has been minimized.

Copy link
Contributor

ulrikstrid commented Feb 26, 2018

100% he beat me to it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment