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

fix: model id type is now boolean|number instead of boolean #2406

Merged
merged 2 commits into from Mar 7, 2019
Merged

fix: model id type is now boolean|number instead of boolean #2406

merged 2 commits into from Mar 7, 2019

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.

None yet

3 participants