Less Nagging #282

xavdid · 2018-03-24T00:17:28Z

Per our conversation, pull out some of the reminding about updating version.

I pulled the custom update method because the user needs to update both CLI and the core dep in each of their apps, which gets a little wordy for an update message. Maybe we should add a section to the doc about what you need to update and when?

eliangcs

I haven't finished reading the code yet, but I have a quick question. Looks like now we check update for zapier-platform-core only when devs run zapier test, is that correct? What's the reason to move the check from zapier upload to zapier test?

xavdid · 2018-03-27T06:24:46Z

Before it happened in upload. That seemed like a weird time to check (maybe because we got all that info back from the server?) so test made more sense to me. Could also be validate, but then you run into the snafu where the extra code in zapier validate doesn't run during build. Happy to move it if you have strong feelings.

eliangcs · 2018-03-27T06:32:28Z

I don't have strong feelings, but there may be some devs not running zapier test at all. Maybe we can put it in zapier build? @BrunoBernardino what's your opinion?

xavdid · 2018-03-27T06:38:52Z

Totally. The only other piece to mention is that updateNotifier only notifies on the second run. That is, the first time it's run, it checks to see if it's out of date and stores the fact that it is. The second time it's run, it notes that fact that it has an update stored and announces it. Here's the stored json:

{
	"optOut": false,
	"lastUpdateCheck": 15264 // shortened to manually trigger a check
}

{
	"optOut": false,
	"lastUpdateCheck": 1522132259312,
	"update": {
		"latest": "5.0.0",
		"current": "4.3.2",
		"type": "major",
		"name": "zapier-platform-core"
	}
}

It's probably fine, but I would be more on the dev running test twice than build. Probably fine either way though!

BrunoBernardino

Very nice cleanup @xavdid !

I do have a few questions which I'd like to see an answer for before approving this, please.

Note I also didn't pull this down yet as it seemed straightforward, but I will before finally approving.

BrunoBernardino · 2018-03-27T07:55:04Z

README.md

@@ -2375,39 +2372,34 @@ source ~/.bash_completion.d/_zapier

 Finally, restart your shell and start hitting TAB with the `zapier` command!

-## Upgrading Zapier Platform CLI or Zapier Platform Core
+## The Zapier Platform Packages


We used to have wording similar to this and then introduced the "separate" version because devs commented that it was confusing (especially because you can use CLI 4.2.0 with core 3.3.0, for example — imagine you've got 2 apps and one is using the latest core and the other isn't). Also because they would update one of the packages but still get a warning (so that's why we were being so explicit).

In any case, most times I agree it will make sense to just update both, since we always keep both "in sync" for versions and don't really test for backwards compatibility.

@ZaneLyon / @bcooksey Thoughts here?

I'm having a little trouble following what you're recommending here. Is the problem with the highlighted line or the content of the section in general?

I think part of ~~our~~ my (don't want to speak for everyone) problem here is that we've worked extensively with these packages (and node in general) so a lot of the necessary actions here seem very obvious. Since CLI and core are updated separately (directly from npm vs editing a file and re-installing) it made more sense to me to thematically break them out a bit. Also thinking about the user with multiple apps, CLI will be updated once but core will be updated repeatedly, so being able to access those separately in the docs made sense.

Sorry that wasn't clear! My comment was related to the content of the section in general.

I don't know if I read something else, or if it was too early when I reviewed this, but the content doesn't seem that much different than what was there before, so please ignore this comment.

I'm very sorry. 🙇

Nothing to apologize for. 😄The goal here was to clean up the language, the content itself was good.

BrunoBernardino · 2018-03-27T07:56:01Z

docs/cli.html

@@ -948,7 +948,7 @@ <h2 id="logs">logs</h2>
 <li><code>--status={any,success,error}</code> -- <em>optional</em>, display only success logs (status code &lt; 400 / info) or error (status code &gt; 400 / tracebacks). Default is <code>any</code></li>
 <li><code>--type={console,http}</code> -- <em>optional</em>, display only console or http logs. Default is <code>console</code></li>
 <li><code>--detailed</code> -- <em>optional</em>, show detailed logs (like request/response body and headers)</li>
-<li><code>--user=user@example.com</code> -- <em>optional</em>, display only this user&apos;s logs. Default is <code>me</code></li>
+<li><a href="mailto:`--user=user@example.com">`--user=user@example.com</a><code>-- _optional_, display only this user&apos;s logs. Default is</code>me`</li>


This link will be awkward, but probably not a big deal. Just wondering if we have a rogue dependency mismatch between some of us.

not sure! I didn't change this, but it popped out in the docs. I just did a rm -rf node_modules && npm i && npm run docs and had no local changes on this branch. the package-lock should guarantee that others have the same result. There's probably something in the markdown packages that are generating the mailto link, which I can fix. not sure where it came from.

No worries, was just wondering.

so I dug in here. seems like it's because litdoc (which generates docs) installs a range and last time the package-lock got cleared, we slid up a version that changed the way they parsed emails (this PR, probably went out in 0.3.14). I agree it looks a little weird, but probably not worth fixing at this point

BrunoBernardino · 2018-03-27T08:00:32Z

src/entry.js

-  }
+  updateNotifier({
+    pkg: require('../package.json'),
+    updateCheckInterval: UPDATE_NOTIFICATION_INTERVAL


The reason why we added this specific message was to make it clearer what needed to be done by the developer (highlighting the problem, command, and remind to re-test).

It was part of a UX research/review, so I'm not so sure we'd want to remove it.

If we found out something more about this in the meanwhile, then it's perfectly fine to change! #recommendation

I like the clear instructions element of it, but I felt compelled to change it because the message was misleading. It said to "run npm i -g zapier-platform-cli and then re-test your integration". Since the test code will break when core is updated (not cli), re-running test there doesn't reflect the new version. A better message might describe updating the dep in package.json and then re-running npm install, but that on top of wanting them to update CLI seemed to get really wordy for a quick "pop up". Also, assuming that they're not updating across a major version, there's really no need to re-test. The semver package allows us to get major|minor|patch from a pair of versions (.difference) so we could selectively include the re-test message if it's important.

Was the concern on the original UX finding that the dev wouldn't know how to update the dependency or wasn't sure what effect doing so would have? I feel like I'm missing some context there. Do you know offhand where the result of those test(s) were?

@xavdid has a point. Maybe we should move the message ("re-test your integration") to where we notify upgrading core?

That was a request by @benzapier and @ZaneLyon IIRC. I didn't check where the data for that request came from, but seemed anecdotal.

I agree the message would be quite large for the whole correct instruction, so maybe @eliangcs 's suggestion makes more sense.

Sweet, grabbed the best of both worlds.

er, might swap the version numbers to after the filename

This is the version i'm pushing:

Thanks, that looks awesome!

BrunoBernardino · 2018-03-27T08:06:43Z

src/utils/misc.js

@@ -151,13 +153,73 @@ const entryPoint = dir => {
  return fse.realpathSync(path.resolve(dir, packageJson.main));
 };

+const printVersionInfo = context => {


I like this helper, nice job! I think it's acceptable to print the versions while doing a push/upload.

BrunoBernardino · 2018-03-27T08:07:02Z

src/utils/api.js

@@ -274,18 +273,8 @@ const upload = (zipPath, appDir) => {
        }
      });
    })
-    .then(appVersion => {
+    .then(() => {
      printDone();


If I'm reading this correctly, we're no longer running this version check on the push/upload, only on version (we also do a different one on test — why?), and we also no longer provide the URL to help update the dependencies, or understand what they are and why they're different.

Adding the check and URL on these commands was made via UX research/review so unless we found out something more, I don't feel very comfortable removing that as it might re-create confusion. #recommendation

Sure, so there's two different (similar) pieces of code being run. in zapier -v, it previously spit out the node version and CLI version. I expanded that to include the depended-on core version (convenient) and figured I'd expand it to do a check for whether the installed version matches the required version. This is simply informational and only run on demand.

Separately, test (but probably soon build per discussion here) checks to see if the required version is behind the most recently published version and once a week (when build is run) it'll notify them that an update is available. The main goal of this PR was to reduce the amount of times we told them they were out of date (per feedback from this typeform). Especially when the code is running fine, there's no particular need to upgrade and us pushing that aggressively devs (core on every upload, daily for CLI) was causing friction.

Ok, that makes sense. Thank you for the explanation. 🙇

I think we'd be more inline with those objectives if we are more explicit on showing them how to update what (like a link to a doc, similar to what we had) on build.

I'm not entirely sold on the benefits of splitting the core vs cli check, but we don't really have a lot of data on it, so I'm fine rolling with it, as you've been more involved in the recent research than I have.

Ah, the reason they need to be in separate spots is because CLI can be run independently of an app. the CLI check can conveniently be put in entry which is run every time the CLI is invoked. We have to attach the check for core somewhere where we know we're working with an app. Your suggestion of build is a good one, since that's probably the most commonly run command.

I see, that sounds like a great plan, then!

BrunoBernardino · 2018-03-27T08:14:27Z

I reviewed without noticing the comments. zapier build they're required to run, zapier test (I saw it for version on cli and core, only core in test, is that correct?) they're only encouraged, so we should keep that important check in "mandatory" commands. #recommendation

eliangcs · 2018-03-27T08:59:26Z

Bruno gave a really thorough review and I totally agree with his comments. In summary, here are what I have in mind:

It's 💯 to check update for cli in entry.js
Maybe remove the check for cli from zapier version (i.e. printVersionInfo)?
I suggest we check update for core only in zapier build and not elsewhere

xavdid · 2018-03-27T09:17:17Z

@eliangcs Sweet. Tomorrow I'll move the updateNotifier code from test into build. Per your other comments, I there might be some confusion between what happens in printVersionInfo and the other places. Per this comment, the code that has updateNotifier is what actually prints the prompt to upgrade to a newer available version (via information gleaned from npm). That (only) happens in entry for cli and test (soon build) for core. The code in printVersionInfo just prints info about what's currently installed/required and doesn't do a check against the server. It's a small expansion on previous -v behavior.

Given that, besides moving code and agreeing on doc content, are there any other action items?

eliangcs

For some reason, I misread printVersionInfo and thought there was an updateNotifier in it. My bad. 🤦‍♂️

Besides moving the check to zapier build, I've read the doc changes more carefully and suggested some changes. They're all #suggestion. Feel free to merge once you feel good about it.

eliangcs · 2018-03-27T09:43:46Z

README-source.md


- [`zapier-platform-cli`](https://github.com/zapier/zapier-platform-cli): This is the `zapier` command. Should be installed with `npm install -g zapier-platform-cli` and is used to interact with your app and Zapier, but doesn't run in nor is included by your app.
+- [`zapier-platform-cli`](https://github.com/zapier/zapier-platform-cli) is the code that powers the `zapier` command. You use it most commaonly with the `test`, `scaffold`, and `push` commands. It's installed with `npm install -g zapier-platform-cli` and does not correspond to a particular app.


commaonly

We have a typo here.

eliangcs · 2018-03-27T09:44:17Z

README-source.md


- [`zapier-platform-core`](https://github.com/zapier/zapier-platform-core): This is what allows your app to interact with Zapier. Apps require this package in their `package.json` file, and a specific version by design, so you can keep using an older "core" version without having to upgrade your code, but still being able to update your app. Should be installed with `npm install` in your app. You should manually keep this version number in sync with your `zapier-platform-cli` global one, but it's not required for all upgrades.
+- [`zapier-platform-core`](https://github.com/zapier/zapier-platform-core) is what allows your app to interact with Zapier. It holds the `z` object and app tester code. Your app depends on a specific version of `zapier-platform-core` in the `package.json` file. It's installed via `npm install` along with the rest of your app's dependencies.


I suggest to change this:

zapier-platform-core is what allows your app to interact with Zapier.

to something like:

zapier-platform-core is the library on which your app depends to interact with Zapier.

eliangcs · 2018-03-27T09:45:23Z

README-source.md


-Check your version with `$ zapier --version` and update with `$ npm -g update zapier-platform-cli`.
+The Zapier platform and its tools are under active development While you don't need to install every release, we release new versions because they are better than the last. We do our best to adhere to [Semantic Versioning](https://semver.org/) wherein we won't break your code unless there's a `major` release. Otherwise, we're just fixing bugs (`patch`) and adding features (`minor`).


... are under active development While ...

Looks like we're missing a period here.

eliangcs · 2018-03-27T09:48:50Z

README-source.md


-### Upgrading Zapier Platform CLI
+### Updating


I feel "upgrading" is a more concrete word and better here. But I'm no native English speaker, so ignore me if I'm wrong.

Honestly I forget that you're not, your English is great. Update and upgrade are pretty interchangeable. I changed it because npm uses it, but ironically it's aliased as upgrade. This answer also says they're pretty much the same.

To me, update feels like it's more for it's replacing an older version, upgrade feels like replacing it with a different, better module (which is an upgrade over the last).

But really, either way.

eliangcs · 2018-03-27T10:01:53Z

README-source.md


-You can also [look at the Changelog](https://github.com/zapier/zapier-platform-cli/blob/master/CHANGELOG.md) to understand what changed between versions.
+Barring unforseen circumstances, all released platform versions will continue to work for the forseeable future. While you never *have* to upgrade your app's platform version, we recommend keeping an eye on the [changelog](https://github.com/zapier/zapier-platform-cli/blob/master/CHANGELOG.md) to see what new features and bux fixes are available.


I like how you edited this paragraph. It's much clearer now. But the term "platform version" doesn't seem super clear to me. And this is the first time we see this term in the doc. Maybe we should change it to something like "package version"? So the whole paragraph becomes:

Barring unforseen circumstances, all released package versions will continue to work for the forseeable future. While you never have to upgrade your app's core package version, we recommend keeping an eye on the changelog ...

Nice! Changed to

never have to upgrade your app's platform-core dependency

BrunoBernardino · 2018-03-27T12:51:41Z

README.md


-#### When to Update CLI
+<!-- TODO: if we decouple releases, change this -->


Is this version manually written here? We should have it as a var of sorts, or link to the releases tab, for example, so we don't have to keep updating this manually and potentially forget about it.

yep, generated with this line just like LAMBDA_VERSION. The todo had to do with if we decouple the versions, we'll want to say

version 1.2.3 of CLI and version 4.5.6 of core are the most recent versions

xavdid · 2018-03-28T07:38:19Z

Ok! Pushed a new version @BrunoBernardino.

Things to test:

run zapier -v in root and in an app directory
edit the file at ~/.config/configstore/update-notifier-zapier-platform-core.json to have an old timestamp and then run zapier build twice in an app directory

BrunoBernardino · 2018-03-28T08:39:10Z

I've checked the changes and they look great! I've got other PT3 duties that would halt me pulling and testing this PR, so @eliangcs will take care of that instead, to ship this sooner.

A workaround for sindresorhus/update-notifier#67. To reproduce the issue, follow these steps: 1. Create an app with an old version of zapier-platform-core in package.json. 2. Run `zapier build` once. 3. Update zapier-platform-core version in package.json to the latest version. 4. Run `zapier build` again. The expected behavior is that the notification shouldn't show, but it did.

To reproduce, run `zapier -v` in a directory containing a package.json which doesn't have zapier-platform-core as dependency.

eliangcs · 2018-03-28T15:19:27Z

@xavdid I pushed some commits to address two bugs I found. You can find the bug description in commit 9e45d10 and f0ce5f0. And 6b0b302 is just a follow-up to f0ce5f0.

Now all looks good! Merging.

From a055940 (add warning about version desync and clean up update checker, 2018-03-26, zapier#282).

xavdid added 2 commits March 23, 2018 17:03

notify less often and not bug on upload

f4d57bb

warn about core updates on test

6b5c8ab

xavdid changed the title ~~[WIP] Less Nagging~~ Less Nagging Mar 26, 2018

xavdid requested a review from eliangcs March 26, 2018 21:22

xavdid added 2 commits March 26, 2018 22:01

update readme concerning versions

218aa40

bump semver dep and print out core version info

cc5c4e9

eliangcs reviewed Mar 27, 2018

View reviewed changes

add warning about version desync and clean up update checker

a055940

BrunoBernardino suggested changes Mar 27, 2018

View reviewed changes

eliangcs approved these changes Mar 27, 2018

View reviewed changes

BrunoBernardino reviewed Mar 27, 2018

View reviewed changes

xavdid added 2 commits March 28, 2018 00:34

move core check and update message and readme

ffeabbf

remove testing code

0771e01

BrunoBernardino approved these changes Mar 28, 2018

View reviewed changes

eliangcs added 4 commits March 28, 2018 22:43

Update Notify: Fix TypeError in printVersionInfo

f0ce5f0

To reproduce, run `zapier -v` in a directory containing a package.json which doesn't have zapier-platform-core as dependency.

Remove repeated comment

654ce59

Update Notify: Fix core version not showing

6b0b302

eliangcs merged commit de54ce0 into master Mar 28, 2018

eliangcs deleted the nag-less branch March 28, 2018 15:20

wking added a commit to wking/zapier-platform-cli that referenced this pull request Jun 28, 2018

src/utils/misc: Fix "epxeriencing" -> "experiencing" typo

b1f48cc

From a055940 (add warning about version desync and clean up update checker, 2018-03-26, zapier#282).

wking mentioned this pull request Jun 28, 2018

src/utils/misc: Fix "epxeriencing" -> "experiencing" typo #338

Merged


		- [`zapier-platform-cli`](https://github.com/zapier/zapier-platform-cli): This is the `zapier` command. Should be installed with `npm install -g zapier-platform-cli` and is used to interact with your app and Zapier, but doesn't run in nor is included by your app.
		- [`zapier-platform-cli`](https://github.com/zapier/zapier-platform-cli) is the code that powers the `zapier` command. You use it most commaonly with the `test`, `scaffold`, and `push` commands. It's installed with `npm install -g zapier-platform-cli` and does not correspond to a particular app.


		- [`zapier-platform-core`](https://github.com/zapier/zapier-platform-core): This is what allows your app to interact with Zapier. Apps require this package in their `package.json` file, and a specific version by design, so you can keep using an older "core" version without having to upgrade your code, but still being able to update your app. Should be installed with `npm install` in your app. You should manually keep this version number in sync with your `zapier-platform-cli` global one, but it's not required for all upgrades.
		- [`zapier-platform-core`](https://github.com/zapier/zapier-platform-core) is what allows your app to interact with Zapier. It holds the `z` object and app tester code. Your app depends on a specific version of `zapier-platform-core` in the `package.json` file. It's installed via `npm install` along with the rest of your app's dependencies.


		Check your version with `$ zapier --version` and update with `$ npm -g update zapier-platform-cli`.
		The Zapier platform and its tools are under active development While you don't need to install every release, we release new versions because they are better than the last. We do our best to adhere to [Semantic Versioning](https://semver.org/) wherein we won't break your code unless there's a `major` release. Otherwise, we're just fixing bugs (`patch`) and adding features (`minor`).


		You can also [look at the Changelog](https://github.com/zapier/zapier-platform-cli/blob/master/CHANGELOG.md) to understand what changed between versions.
		Barring unforseen circumstances, all released platform versions will continue to work for the forseeable future. While you never have to upgrade your app's platform version, we recommend keeping an eye on the [changelog](https://github.com/zapier/zapier-platform-cli/blob/master/CHANGELOG.md) to see what new features and bux fixes are available.


		#### When to Update CLI
		<!-- TODO: if we decouple releases, change this -->

Less Nagging #282

Less Nagging #282

Conversation

xavdid commented Mar 24, 2018

eliangcs left a comment • edited Loading

Choose a reason for hiding this comment

xavdid commented Mar 27, 2018 • edited Loading

eliangcs commented Mar 27, 2018

xavdid commented Mar 27, 2018

BrunoBernardino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoBernardino Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavdid Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoBernardino Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoBernardino commented Mar 27, 2018 • edited Loading

eliangcs commented Mar 27, 2018

xavdid commented Mar 27, 2018

eliangcs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavdid Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

xavdid commented Mar 28, 2018

BrunoBernardino commented Mar 28, 2018

eliangcs commented Mar 28, 2018

eliangcs left a comment •

edited

Loading

xavdid commented Mar 27, 2018 •

edited

Loading

BrunoBernardino Mar 27, 2018 •

edited

Loading

xavdid Mar 27, 2018 •

edited

Loading

BrunoBernardino Mar 27, 2018 •

edited

Loading

BrunoBernardino commented Mar 27, 2018 •

edited

Loading

xavdid Mar 28, 2018 •

edited

Loading