Skip to content

Conversation

@marvinirwin
Copy link
Contributor

model id type is now boolean|number instead of boolean

#2245 (comment)

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@raymondfeng
Copy link
Contributor

@marvinirwin At least my intention was to discontinue the usage of number for compound keys. A better way is to do so at model class level (to be implemented), such as:

@model({id: ['p1', 'p2']})

WDYT?

@marvinirwin marvinirwin mentioned this pull request Feb 19, 2019
7 tasks
@marvinirwin
Copy link
Contributor Author

@raymondfeng I made this at the request of @bajtos

see #2245 (comment)

@bajtos
Copy link
Member

bajtos commented Mar 5, 2019

At least my intention was to discontinue the usage of number for compound keys. A better way is to do so at model class level (to be implemented).

In the pull request #2245, @marvinirwin is implementing lb4 discovery command. As I understand the situation, juggler and/or connectors are returning a numeric value for the id flag of discovered model properties. See https://github.com/strongloop/loopback-next/pull/2245/files#r255073664

I proposed to fix our type definitions to allow both boolean & number values for the id flag, to make this compatible with the underlying juggler implementation.

I understand that you @raymondfeng want to move LB4+ in a different direction. What do you propose as a short-term solution to allow lb4 discovery to emit correct TypeScript model files? Are you fine with converting the id value from a number to a boolean, loosing any information about composite primary keys along the way?

This is the currently proposed implementation:

  // o.id could be 1 or 0, typescript will be angry if it is not a boolean
  if (o.id) {
    o.id = !!o.id;
  }

@bajtos bajtos added the Repository Issues related to @loopback/repository package label Mar 5, 2019
@raymondfeng
Copy link
Contributor

I'm fine to allow number for now and worry about it when we rewrite juggler.

@bajtos bajtos merged commit 71292e9 into loopbackio:master Mar 7, 2019
@bajtos
Copy link
Member

bajtos commented Mar 7, 2019

Landed, thank you @marvinirwin for the contribution 🎉

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

Labels

Repository Issues related to @loopback/repository package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants