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

Break everything we can pre-v3 #407

Merged
merged 6 commits into from Oct 2, 2017

Conversation

Projects
None yet
3 participants
@pimterry
Member

pimterry commented Sep 28, 2017

API v3 is an excellent excuse to bring it lots of important but breaking changes I've been putting off for a while. This PR works on the v2 API but changes behaviour by:

  • Remove lots of our model add-ons
    • These aren't currently reliable, as they mean you get different objects depending on whether you expand an object or query it directly. Really this logic should live in the API, but right now it's not strictly necessary at all (it's only used in the CLI afaik), so let's just kill it
    • This is also necessitated by the next step, which is hard to do while keeping these properties
  • Stop expanding relationships by default
    • This is going to significantly reduce unnecessary load on the API, by letting the UI do much cheaper queries.
    • getApplicationName is now the only expand: left in the codebase, and I think it's reasonable and unavoidable.

This is another PR in the queue for SDK v7, so no versionbot, and merging to the v7 branch instead of master until we're ready to ship.

@pimterry pimterry changed the title from WIP: Break everything we can pre-v3 to Break everything we can pre-v3 Sep 29, 2017

@pimterry pimterry requested review from thgreasi and Page- Sep 29, 2017

@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Sep 29, 2017

Member

@Page- I've mostly added you to this PR just because it'll make you happy: since we're breaking the SDK's API to support API v3, I'm removing all the automatic expands everywhere too, which should make a world of difference.

Member

pimterry commented Sep 29, 2017

@Page- I've mostly added you to this PR just because it'll make you happy: since we're breaking the SDK's API to support API v3, I'm removing all the automatic expands everywhere too, which should make a world of difference.

@thgreasi

This comment has been minimized.

Show comment
Hide comment
@thgreasi

thgreasi Sep 29, 2017

Member

Unbelievable title 🤣

Member

thgreasi commented Sep 29, 2017

Unbelievable title 🤣

*BREAKING*: Remove application.online_devices and application.device_…
…length

These were slightly convenient, but hacky and inconsistent with any data
retrieved manually with $expands. One day we might bring these back, but
for now it's much cleaner for everybody if we keep this simple.

Change-Type: major
@@ -117,9 +117,6 @@ getDeviceModel = (deps, opts) ->
return url.resolve(dashboardUrl, "/apps/#{options.appId}/devices/#{options.deviceId}/summary")
addExtraInfo = (device) ->
# TODO: Move this to the server
device.application_name = device.application[0].app_name
device.dashboard_url = getDashboardUrl({ appId: device.application[0].id, deviceId: device.id })

This comment has been minimized.

@Page-

Page- Sep 29, 2017

Contributor

Strictly speaking the ui doesn't give a damn about the app id in dashboard, and using the wrong app id will still get you the right device (something that I abuse fairly often when I know the device id but not app id) - with that in mind it might be possible to give a url direct to a device that the dashboard can understand, cc @LucianBuzzo

@Page-

Page- Sep 29, 2017

Contributor

Strictly speaking the ui doesn't give a damn about the app id in dashboard, and using the wrong app id will still get you the right device (something that I abuse fairly often when I know the device id but not app id) - with that in mind it might be possible to give a url direct to a device that the dashboard can understand, cc @LucianBuzzo

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

This was useful to support agents that used the CLI to retrieve the full dashboard url for the device.
Opened resin-io/resin-ui#1351 for this.
Would it make sense to add the new url when it's ready?

@thgreasi

thgreasi Oct 2, 2017

Member

This was useful to support agents that used the CLI to retrieve the full dashboard url for the device.
Opened resin-io/resin-ui#1351 for this.
Would it make sense to add the new url when it's ready?

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

True. I don't think that matters much for this though - for now it's only used in the CLI, which is going to want to expand the app anyway, so I'll just extract the same logic unchanged to there and keep things simple. In a multi-app future, we'll definitely want device only URLs, but for now I'm not too worried.

@pimterry

pimterry Oct 2, 2017

Member

True. I don't think that matters much for this though - for now it's only used in the CLI, which is going to want to expand the app anyway, so I'll just extract the same logic unchanged to there and keep things simple. In a multi-app future, we'll definitely want device only URLs, but for now I'm not too worried.

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

The UI will also need a getDashboardUrl method for resin-io/resin-ui#1351 .
Would it make sense to change that method to the suggested url and make this PR depend on the UI ticket?

@thgreasi

thgreasi Oct 2, 2017

Member

The UI will also need a getDashboardUrl method for resin-io/resin-ui#1351 .
Would it make sense to change that method to the suggested url and make this PR depend on the UI ticket?

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

I imagine the UI wouldn't want a URL method anyway, since it can use internal routing mechanisms instead (tbc, no idea what our routing will look like in React land). And to be clear, the CLI definitely will keep this functionality, it's just where the logic lives that'll be changing!

@pimterry

pimterry Oct 2, 2017

Member

I imagine the UI wouldn't want a URL method anyway, since it can use internal routing mechanisms instead (tbc, no idea what our routing will look like in React land). And to be clear, the CLI definitely will keep this functionality, it's just where the logic lives that'll be changing!

@@ -117,9 +117,6 @@ getDeviceModel = (deps, opts) ->
return url.resolve(dashboardUrl, "/apps/#{options.appId}/devices/#{options.deviceId}/summary")
addExtraInfo = (device) ->
# TODO: Move this to the server
device.application_name = device.application[0].app_name

This comment has been minimized.

@Page-

Page- Sep 29, 2017

Contributor

This is also nice for a multi-app future 👍

@Page-

Page- Sep 29, 2017

Contributor

This is also nice for a multi-app future 👍

Show outdated Hide outdated lib/models/device.coffee
Show outdated Hide outdated lib/models/device.coffee
@@ -770,7 +766,7 @@ getDeviceModel = (deps, opts) ->
.then ({ application, device }) ->
if device.device_type isnt application.device_type
throw new Error("Incompatible application: #{applicationNameOrId}")
throw new errors.ResinInvalidDeviceType("Incompatible application: #{applicationNameOrId}")

This comment has been minimized.

@Page-

Page- Sep 29, 2017

Contributor

Should this be backported?

@Page-

Page- Sep 29, 2017

Contributor

Should this be backported?

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

It's a bit debatable. I think there are a few weird cases where this could break your code (if you're catching ResinInvalidDeviceType specifically for some other reason, expecting move to fail differently). That's a bit tenuous, but there's no desperate need to backport things like this, so I'd rather bundle it into the big clear break just to be safe.

@pimterry

pimterry Oct 2, 2017

Member

It's a bit debatable. I think there are a few weird cases where this could break your code (if you're catching ResinInvalidDeviceType specifically for some other reason, expecting move to fail differently). That's a bit tenuous, but there's no desperate need to backport things like this, so I'd rather bundle it into the big clear break just to be safe.

@@ -191,15 +186,14 @@ getApplicationModel = (deps, opts) ->
$eq: [
$tolower: $: 'app_name'
appName
]
expand:
],
user:

This comment has been minimized.

@Page-

Page- Sep 29, 2017

Contributor

This can be backported right?

@Page-

Page- Sep 29, 2017

Contributor

This can be backported right?

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

I don't think it can? Before we were expanding user, so you'd get it in your result (but only the id), now we're not expanding it at all.

@pimterry

pimterry Oct 2, 2017

Member

I don't think it can? Before we were expanding user, so you'd get it in your result (but only the id), now we're not expanding it at all.

*BREAKING*: Remove device.application_name and device.dashboard_url
Both of these were generated from expanded data we'd like to remove, and
somewhat messily at that. You can easily generate them by hand by
expanding application yourself and looking at the name and
id there by hand.

Change-Type: major
@thgreasi

thgreasi approved these changes Oct 2, 2017 edited

Beyond the getDashboardUrl question, LGTM 👍 💥

@@ -191,15 +186,14 @@ getApplicationModel = (deps, opts) ->
$eq: [
$tolower: $: 'app_name'
appName
]
expand:

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

To be honest I would expect that a mehtod named getAppWithOwner would also include/expand the user of the app.
On the other hand the summary of the method doesn't mention anything like that, so I guess it's ok.

@thgreasi

thgreasi Oct 2, 2017

Member

To be honest I would expect that a mehtod named getAppWithOwner would also include/expand the user of the app.
On the other hand the summary of the method doesn't mention anything like that, so I guess it's ok.

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

Good point! To solve this, let's just rename the method 😃. It's now getAppByOwner. Clearer?

@pimterry

pimterry Oct 2, 2017

Member

Good point! To solve this, let's just rename the method 😃. It's now getAppByOwner. Clearer?

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

👍

@thgreasi
@@ -191,15 +186,14 @@ getApplicationModel = (deps, opts) ->
$eq: [
$tolower: $: 'app_name'
appName
]
expand:
],

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

I guess that we don't need this , but I'm not sure whether it's required by the our linter.

@thgreasi

thgreasi Oct 2, 2017

Member

I guess that we don't need this , but I'm not sure whether it's required by the our linter.

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

Our linter doesn't care (it gets run automatically during builds, so if it did care, it'd fail the PR). We're going to rewrite this into TS and add prettier etc soon-ish anyway, I'm not too worried about minor style points either way.

@pimterry

pimterry Oct 2, 2017

Member

Our linter doesn't care (it gets run automatically during builds, so if it did care, it'd fail the PR). We're going to rewrite this into TS and add prettier etc soon-ish anyway, I'm not too worried about minor style points either way.

@@ -117,9 +117,6 @@ getDeviceModel = (deps, opts) ->
return url.resolve(dashboardUrl, "/apps/#{options.appId}/devices/#{options.deviceId}/summary")
addExtraInfo = (device) ->
# TODO: Move this to the server
device.application_name = device.application[0].app_name
device.dashboard_url = getDashboardUrl({ appId: device.application[0].id, deviceId: device.id })

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

This was useful to support agents that used the CLI to retrieve the full dashboard url for the device.
Opened resin-io/resin-ui#1351 for this.
Would it make sense to add the new url when it's ready?

@thgreasi

thgreasi Oct 2, 2017

Member

This was useful to support agents that used the CLI to retrieve the full dashboard url for the device.
Opened resin-io/resin-ui#1351 for this.
Would it make sense to add the new url when it's ready?

@@ -229,9 +229,7 @@ getEnvironmentVariablesModel = (deps, opts) ->
return pine.get

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

Would it make sense to use isId to check whether we actually need to do the request?
Would it make sense to expand the device_environment_variables in the deviceModel().get call so that we only have a single request in all cases?

@thgreasi

thgreasi Oct 2, 2017

Member

Would it make sense to use isId to check whether we actually need to do the request?
Would it make sense to expand the device_environment_variables in the deviceModel().get call so that we only have a single request in all cases?

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

There's a few cases like this throughout the SDK, where we always make the upfront request for the id, even though we might not need to. They exist because we want to consistently fail with a missing device error if the device doesn't exist. If we only make the single request below (to look up env vars for the device id) then we can't tell the difference between "the device doesn't exist" and "the device has no env vars". I'd like to avoid that in future, but unless there's an easy answer I'm planning to ignore it for now.

@pimterry

pimterry Oct 2, 2017

Member

There's a few cases like this throughout the SDK, where we always make the upfront request for the id, even though we might not need to. They exist because we want to consistently fail with a missing device error if the device doesn't exist. If we only make the single request below (to look up env vars for the device id) then we can't tell the difference between "the device doesn't exist" and "the device has no env vars". I'd like to avoid that in future, but unless there's an easy answer I'm planning to ignore it for now.

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

How about expanding the device_environment_variables through the deviceModel().get call?
That should throw the "the device doesn't exist" error properly? It might be an overkill though and I'm happy with the current implementation.

@thgreasi

thgreasi Oct 2, 2017

Member

How about expanding the device_environment_variables through the deviceModel().get call?
That should throw the "the device doesn't exist" error properly? It might be an overkill though and I'm happy with the current implementation.

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

Hmmm, yes, you're probably right that there's a route through there (though it may be messy). I don't want to worry about it right now, but I've filed an issue: #409

@pimterry

pimterry Oct 2, 2017

Member

Hmmm, yes, you're probably right that there's a route through there (though it may be messy). I don't want to worry about it right now, but I've filed an issue: #409

@@ -128,8 +128,6 @@ exports.mergePineOptions = (defaults, extras) ->
if value?
if not isArray(value)
value = [value]
if !includes(value, 'id')

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

Oh so now we will have to explicitly include the ID 👍 More deterministic result object structure is on the way :)

@thgreasi

thgreasi Oct 2, 2017

Member

Oh so now we will have to explicitly include the ID 👍 More deterministic result object structure is on the way :)

This comment has been minimized.

@pimterry

pimterry Oct 2, 2017

Member

Yep, it was just there before because lots of the object normalisation stuff needed it - now that's gone, this can go too :-)

@pimterry

pimterry Oct 2, 2017

Member

Yep, it was just there before because lots of the object normalisation stuff needed it - now that's gone, this can go too :-)

@@ -117,9 +117,6 @@ getDeviceModel = (deps, opts) ->
return url.resolve(dashboardUrl, "/apps/#{options.appId}/devices/#{options.deviceId}/summary")
addExtraInfo = (device) ->
# TODO: Move this to the server
device.application_name = device.application[0].app_name
device.dashboard_url = getDashboardUrl({ appId: device.application[0].id, deviceId: device.id })

This comment has been minimized.

@thgreasi

thgreasi Oct 2, 2017

Member

The UI will also need a getDashboardUrl method for resin-io/resin-ui#1351 .
Would it make sense to change that method to the suggested url and make this PR depend on the UI ticket?

@thgreasi

thgreasi Oct 2, 2017

Member

The UI will also need a getDashboardUrl method for resin-io/resin-ui#1351 .
Would it make sense to change that method to the suggested url and make this PR depend on the UI ticket?

pimterry added some commits Sep 28, 2017

@pimterry pimterry merged commit 65e82fd into v7 Oct 2, 2017

6 checks passed

AutoMerges Manual merging is in effect for this PR
Reviewers 1/1 review approvals met
Versionist Versioning for this PR is disabled
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@pimterry pimterry deleted the breaking-improvements branch Oct 2, 2017

pimterry added a commit that referenced this pull request Oct 11, 2017

pimterry added a commit that referenced this pull request Oct 12, 2017

@pimterry pimterry referenced this pull request Oct 12, 2017

Merged

Release Resin-SDK v7.0.0 #420

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