Support setting Alfred variables with alfy.output (#43) #44

luca-aurelia · 2017-01-12T15:16:10Z

Linter and tests (including new ones) are all green. Also updated the readme. I tried to match the existing line usage and code organization. I'd love feedback. I'm just getting started with open source contributions, and I know there's plenty to learn.

SamVerschueren · 2017-03-08T07:49:27Z

Hi @noise-machines, thank you for the PR! Really appreciate new contributions :).

I tried this out first by looking how you pass down environment variables. It doesn't look like what you're trying to do is supported. This is what I tried and it looks like you are trying to do the same thing

alfy.output([
    title: 'Hello World',
    arg: JSON.stringify({arg: 'foo', variables: {foo: 'bar'}})
]);

I executed this but the only thing I get is just the stringified object as argument. It's not being parsed.

So I dived into the documentation and found this page and more specifically, the Variables / Session Variables section. It just mentions variables as a global output but not on a per item basis. Do you happen to have proof that this should work?

luca-aurelia · 2017-03-08T15:36:48Z

Hi @SamVerschueren,

Thanks for the feedback! I discovered this feature through this excellent forum post. The trick is to stringify an object that has an alfredworkflow key. It's easy to miss, but I do that here in my PR. Your code will work if you change it to:

alfy.output([{
  title: 'Hello World',
  arg: JSON.stringify({
    alfredworkflow: { arg: 'foo', variables: { foo: 'bar' } }
  })
}]);

Here's a demo of it working on my machine with Alfred 3.3.1:

If you think it'd be helpful, I can update my PR to include references to that forum post where appropriate.

SamVerschueren · 2017-03-08T16:00:48Z

Aha, alright. Let me check it later today! Thanks again for working on it, it's pretty cool stuff.

SamVerschueren

Overall looks good! Works great and I actually need this in one of my workflows, didn't know it was possible :).

Added some comments, just push the changes on the same branch and it will automatically pick it up.

SamVerschueren · 2017-03-08T20:39:58Z

index.js

+};
+
+alfy.output = items => {
+	if (isDefined(items) && items.forEach) {


Just check if items is an array, otherwise throw a TypeError

if (!Array.isArray(items)) { throw new TypeError(`Expected \`items\` to be of type \`Array\`, got \`${typeof items}\``); }

This would be a breaking change to the output API, right?

Old implementation:

alfy.output('hi!'); // no problem

New implementation:

alfy.output('hi!'); // TypeError

I'm happy to implement it, just want to confirm.

P.S. - does Alfy follow SemVer?

For now, I've just changed it to this:

if (Array.isArray(items)) { items = items.map(format); }

Let me know if you'd prefer the breaking change.

It was always documented as accepting an Array. Enforcing the documented behavior is not a breaking change.

I agree. I don't think Alfred accepts anything else except an Array of items. This means that we are now just more verbose to what it accepts.

SamVerschueren · 2017-03-08T20:41:21Z

readme.md

+Using the `variables` property:
+
+```js
+alfy.output([{


Put the objects on newlines

alfy.output([ { title: 'Unicorn', arg: '🦄', variables: { color: 'white' } }, { //... } ]);

SamVerschueren · 2017-03-08T20:42:36Z

readme.md

+//=> 'myriad' if they selected Rainbow
+```
+
+Alfred Workflow Variables are also available in the workflow editor using the form `{var:varname}`.


Alfred Workflow Variables are also available in the workflow editor using the form {var:varname}.

Can you add an example for the color case above? Would {myColor:color} work for instance?

SamVerschueren · 2017-03-08T20:43:25Z

index.js

+	if (isDefined(items) && items.forEach) {
+		items.forEach(formatItemArg);
+	}
+


Use items.map

items = items.map(item => format(item));

And then return the item in the format function.

SamVerschueren · 2017-03-08T20:46:03Z

index.js

+const isDefined = x => x !== null && x !== undefined;
+
+const formatItemArg = item => {
+	if (isDefined(item) && isDefined(item.variables)) {


This has to be expanded. It's also possible to provide variables for mods like this

alfy.output([ { title: 'Unicorn', arg: '🦄', variables: { color: 'white' }, mods: { alt: { title: 'Rainbow', arg: '🌈', variables: { color: 'red' } } } } ]);

If you press the alt key, it will now show the Rainbow instead.

SamVerschueren · 2017-03-08T20:47:54Z

test/output.js

@@ -0,0 +1,76 @@
+import test from 'ava';


Shouldn't this be import {serial as test} from 'ava'; to make sure it runs all the tests serially. Otherwise the output could be mangled. It's weird that it works right now to be honest.

- handle mod keys - implement tests for mod keys - check if items is an array instead of checking existence of items.forEach - change items.forEach to items.map - fix object formatting in readme - add example of referencing an environment variable in the workflow editor - run tests serially

luca-aurelia · 2017-03-09T03:08:42Z

Thanks for the excellent and in-depth feedback, @SamVerschueren! I believe I got all your requested changes. Let me know if there's anything else I can do.

SamVerschueren

Sorry for all the comments. I like to review in multiple phases :).

Feel free to discuss some of my comments. Maybe you did some things because of a reason. Don't be afraid to say so.

I think we should also put those new format function in a lib/util.js file.

I was wondering earlier today if we should call it env instead of variables. Any suggestions or feedback on this?

// @sindresorhus

SamVerschueren · 2017-03-09T06:32:49Z

index.js

+
+alfy.output = items => {
+	if (Array.isArray(items)) {
+		items = items.map(format);


It's better to use items.map(item => format(item)). The reason for this is that the map callback returns multiple parameters. This results in sending all those parameters to format which may result in bugs in the future if for instance format starts excepting additional input arguments.

Ah, didn't think about that! Good point. Fixed 😄

SamVerschueren · 2017-03-09T06:36:45Z

readme.md

@@ -179,6 +179,40 @@ alfy.output([{

 <img src="media/screenshot-output.png" width="694">

+Using the `variables` property:


I think we should put this under it's own section so we can easily link it from the highlights and to make it more clear. Not sure where though, any ideas @sindresorhus? Maybe a new section Environment variables under Usage or Example? Or on the same level as those?

A new top-level section called "Environment variables".

SamVerschueren · 2017-03-09T06:37:10Z

index.js

+};
+
+alfy.output = items => {
+	if (Array.isArray(items)) {


To come back to the throw new TypeError() thing. Just wondering, is there a reason why you would use alfy.output without providing an Array? If not, I think I like to be more explicit about it and throw an error instead.

// @sindresorhus

Did a little more research into this question, and I agree with you about throwing the TypeError, @SamVerschueren. I was misremembering the previous behavior of alfy.output. It already assumes you've passed an Array:

// previous implementation alfy.output = arr => { console.log(JSON.stringify({items: arr}, null, '\t')); };

It's true that the previous implementation wouldn't throw a TypeError, but I think the error is actually better. It prevents people from doing stuff like

alfy.output('hi!'); // outputs '{"items":"hi!"}'

which doesn't make sense.

If @sindresorhus agrees, let's move forward with throwing a TypeError.

I agree, should throw a TypeError.

SamVerschueren · 2017-03-09T06:40:02Z

index.js

+// see https://www.alfredforum.com/topic/9070-how-to-workflowenvironment-variables/
+// for documentation on setting environment variables from Alfred workflows
+const format = item => {
+	if (!isDefined(item)) {


I think if item is not defined, we should make it an empty object instead.

item = item || {};

And just continue. This will make sure that we don't output null or undefined to Alfred which will probably result in a crash.

I tried outputting an array of nulls and noticed something pretty interesting. If you're calling your script from a script filter, Alfred just hangs when you output an array of non-objects.

// Example 1: Alfred hangs when this is called from a script filter alfy.output(['hi', 'hey']); // Example 2: same for this alfy.output([null, null, undefined]);

When called from a Run Script action, you just get stringified JSON as your Alfred {query}:

'{ "items": [ null, null ] }'

How does that influence our decision here? Not quite sure. If we use

item = item || {};

that will protect users from example 2 above, but not from example 1. What do you think?

I would just throw if it's a non-object. Better to fail loudly than being silently "smart".

Could use: https://github.com/sindresorhus/is-plain-obj

SamVerschueren · 2017-03-09T06:40:08Z

index.js

+		return item;
+	}
+
+	if (isDefined(item.variables)) {


Just write if (item.variables) {

SamVerschueren · 2017-03-09T06:41:15Z

index.js

+		item = wrapArg(item);
+	}
+
+	if (isDefined(item.mods)) {


if (item.mods) {

SamVerschueren · 2017-03-09T06:42:49Z

index.js

+};
+
+const formatMods = item => {
+	const copy = Object.assign({}, item);


I don't think we need to copy it first. It doesn't really matter in this case if we alter the object or not.

https://github.com/sindresorhus/alfy/pull/44/files#r106093156

SamVerschueren · 2017-03-09T06:43:59Z

index.js

+const formatMods = item => {
+	const copy = Object.assign({}, item);
+
+	Object.keys(item.mods).forEach(modKey => {


We are more fan of for...of :)

for (const mod of Object.keys(item.mods)) { item.mods[mod] = wrapArg(item.mods[mod]); } return item;

Great! That syntax looks a little nicer to me too. I was wondering myself whether to go with the loop body you suggested.

Done 😄

SamVerschueren · 2017-03-09T06:45:52Z

index.js

+const wrapArg = item => {
+	const alfredworkflow = {arg: item.arg, variables: item.variables};
+	const arg = JSON.stringify({alfredworkflow});
+	const newItem = Object.assign({}, item, {arg});


Just put argument on the item object directly. No need to copy it I guess.

I see your reasoning. But I copied the item because I wanted to avoid this kind of scenario:

const unicorn = { title: 'unicorn', arg: '🦄', variables: { selectedMagicalBeing: true } }; alfy.output([unicorn]); const unicornEmoji = unicorn.arg; doSomething(unicornEmoji); // weird behavior. unicornEmoji is now a stringified JSON object

Obviously, you can avoid this sort of problem by calling alfy.output last. And I'm sure that's how most people use it. I just don't want Alfy users to have to remember to call alfy.output last.

General rule is to always use Object.assign if you modify a user-specified object. Not doing so can cause subtle bugs.

SamVerschueren · 2017-03-09T06:46:54Z

test/output.js

+	t.notThrows(() => m.output(undefined));
+});
+
+const itemWithMod = {


Put this at the top.

luca-aurelia · 2017-03-09T13:55:40Z

I like calling it env instead of variables. You use env when accessing the environment variables, so you should use env when setting them.

SamVerschueren · 2017-03-09T15:30:52Z

Yes to me it sounds much better then variables. Don't like to decide this alone though, like to have @sindresorhus his opinion as well.

sindresorhus · 2017-03-15T06:25:49Z

index.js

+const wrapArg = item => {
+	const alfredworkflow = {arg: item.arg, variables: item.variables};
+	const arg = JSON.stringify({alfredworkflow});
+	const newItem = Object.assign({}, item, {arg});


General rule is to always use Object.assign if you modify a user-specified object. Not doing so can cause subtle bugs.

sindresorhus · 2017-03-15T06:25:58Z

index.js

+};
+
+const formatMods = item => {
+	const copy = Object.assign({}, item);


https://github.com/sindresorhus/alfy/pull/44/files#r106093156

sindresorhus · 2017-03-15T06:27:39Z

index.js

+// see https://www.alfredforum.com/topic/9070-how-to-workflowenvironment-variables/
+// for documentation on setting environment variables from Alfred workflows
+const format = item => {
+	if (!isDefined(item)) {


I would just throw if it's a non-object. Better to fail loudly than being silently "smart".

Could use: https://github.com/sindresorhus/is-plain-obj

sindresorhus · 2017-03-15T06:30:13Z

readme.md

@@ -179,6 +179,40 @@ alfy.output([{

 <img src="media/screenshot-output.png" width="694">

+Using the `variables` property:


A new top-level section called "Environment variables".

sindresorhus · 2017-03-15T06:31:51Z

readme.md

+
+```js
+// After a user selects "Unicorn" or "Rainbow"
+process.env.color


Should we alias this as alfy.env?

Yes, that would be nice.

sindresorhus · 2017-03-15T06:33:30Z

Yes, let's call it env and not variables.

SamVerschueren · 2017-04-02T10:47:51Z

@noise-machines ping :)

luca-aurelia · 2017-04-03T02:40:11Z

Thanks for the ping @SamVerschueren. Will implement the desired changes this week. :)

SamVerschueren · 2017-04-21T19:32:24Z

@noise-machines Sorry for another ping :p. If you don't find the time to finish this, give me write access to your repository and I might be able to do this for you. Or if you want to do it yourself, feel free :).

luca-aurelia · 2017-04-25T14:04:41Z

@SamVerschueren, no worries! If I don't get to it in the next few days, I'll give you write access.

sindresorhus · 2017-06-26T15:59:27Z

@noise-machines Interested in finishing this up? If not, could you give write-access to me and Sam?

luca-aurelia · 2017-07-14T06:35:28Z

Okay, I believe all requested changes have been implemented. Thanks for your patience and excellent feedback, @SamVerschueren and @sindresorhus.

SamVerschueren

Sorry for the late reply, forgot about this PR. Let's get it merged :).

Have some minor improvements. I also think we should extract the format (+ all the other extra methods) to a lib/utils.js file.

SamVerschueren · 2017-07-15T12:26:53Z

index.js

+// for documentation on setting environment variables from Alfred workflows.
+const format = item => {
+	if (!isPlainObj(item)) {
+		throw new TypeError(`Expected \`item\` to be a plain object, got ${typeof item}.`);


Wrap ${typeof item} between backticks as well.

SamVerschueren · 2017-09-21T05:52:59Z

index.js

+};
+
+// See https://www.alfredforum.com/topic/9070-how-to-workflowenvironment-variables/
+// for documentation on setting environment variables from Alfred workflows.


Put the comment on one line

SamVerschueren · 2017-09-21T05:53:53Z

index.js

+
+alfy.output = items => {
+	if (!Array.isArray(items)) {
+		throw new TypeError(`Expected \`items\` to be an Array, got ${typeof items}`);


Wrap ${typeof items} between backticks as well.

SamVerschueren · 2017-09-21T05:55:27Z

readme.md

+  {
+    title: 'Unicorn',
+    arg: '🦄',
+    variables: {


SamVerschueren · 2017-09-21T05:55:36Z

readme.md

+  {
+    title: 'Rainbow',
+    arg: '🌈',
+    variables: {


luca-aurelia · 2017-09-28T01:06:41Z

Hey @SamVerschueren, latest commits should implement those changes. Let me know if there's anything else!

SamVerschueren · 2017-09-28T05:50:00Z

index.js

@@ -9,6 +9,7 @@ const cleanStack = require('clean-stack');
 const dotProp = require('dot-prop');
 const CacheConf = require('cache-conf');
 const updateNotification = require('./lib/update-notification');
+const {format} = require('./lib/utils');


Destructuring is not yet supported in Node 4. So just import it as utils and use utils.format below.

SamVerschueren · 2017-09-28T05:51:47Z

index.js

@@ -159,6 +165,8 @@ alfy.icon = {
 	delete: getIcon('ToolbarDeleteIcon')
 };

+alfy.env = process.env;


@sindresorhus Should we put this in alfy.meta.env instead?

SamVerschueren · 2017-09-28T05:51:55Z

lib/utils.js

@@ -0,0 +1,36 @@
+const isPlainObj = require('is-plain-obj');


'use strict';

SamVerschueren · 2017-09-28T05:53:31Z

lib/utils.js

+	const alfredworkflow = {arg: item.arg, variables: item.env};
+	const arg = JSON.stringify({alfredworkflow});
+	const newItem = Object.assign({}, item, {arg});
+	delete newItem.env;


Can we overwrite env with undefined instead in the line above? If I'm not mistaken delete is bad for performance.

SamVerschueren · 2017-09-28T05:55:32Z

lib/utils.js

+};
+
+// See https://www.alfredforum.com/topic/9070-how-to-workflowenvironment-variables/ for documentation on setting environment variables from Alfred workflows.
+module.exports.format = item => {


exports.format

SamVerschueren · 2017-09-28T05:56:10Z

package.json

    "cache-conf": "^0.3.0",
-    "clean-stack": "^1.0.0",
+    "clean-stack": "^1.3.0",


SamVerschueren · 2017-09-28T05:56:15Z

package.json

    "conf": "^0.11.0",
-    "dot-prop": "^4.0.0",
+    "dot-prop": "^4.1.1",


SamVerschueren · 2017-09-28T05:57:13Z

readme.md

@@ -165,7 +166,12 @@ Return output to Alfred.

 Type: `Array`

-List of `Object` with any of the [supported properties](https://www.alfredapp.com/help/workflows/inputs/script-filter/json/).
+List of `Object` with any of the [supported properties](https://www.alfredapp.com/help/workflows/inputs/script-filter/json/). If a list item has a `variables` property, it will be used to set Alfred's [Workflow Environment Variables](https://www.alfredapp.com/help/workflows/advanced/variables/) when the user selects the item.


If a list item has a `env` property...

SamVerschueren · 2017-09-28T05:57:47Z

test/output.js

+const itemWithMod = {
+	title: 'unicorn',
+	arg: '🦄',
+	env: {fabulous: true},


Put it on a new line

SamVerschueren · 2017-09-28T05:59:20Z

test/output.js

+};
+
+const m = alfy();
+const hook = cb => {


Return a promise instead of a callback, this way you can just use async/await in your tests.

sindresorhus · 2019-06-10T17:27:42Z

Closing for lack of activity.

Support setting Alfred variables with alfy.output (sindresorhus#43)

979febc

SamVerschueren requested changes Mar 8, 2017

View reviewed changes

SamVerschueren requested changes Mar 9, 2017

View reviewed changes

sindresorhus requested changes Mar 15, 2017

View reviewed changes

Implement @sindresorhus' requested changes.

76de28b

SamVerschueren requested changes Sep 21, 2017

View reviewed changes

C. Thomas Bailey added 2 commits September 27, 2017 21:03

Fix formatting as requested by @SamVerschueren.

7f4ac51

Fix additional formatting.

fecf997

SamVerschueren requested changes Sep 28, 2017

View reviewed changes

sindresorhus closed this Jun 10, 2019

sindresorhus mentioned this pull request Jun 10, 2019

Support setting Alfred variables with alfy.output #43

Open

		@@ -179,6 +179,40 @@ alfy.output([{

		<img src="media/screenshot-output.png" width="694">

		Using the `variables` property:

Support setting Alfred variables with alfy.output (#43) #44

Support setting Alfred variables with alfy.output (#43) #44

Conversation

luca-aurelia commented Jan 12, 2017

SamVerschueren commented Mar 8, 2017

luca-aurelia commented Mar 8, 2017

SamVerschueren commented Mar 8, 2017

SamVerschueren 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca-aurelia commented Mar 9, 2017 • edited

SamVerschueren 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca-aurelia commented Mar 9, 2017

SamVerschueren commented Mar 9, 2017

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

sindresorhus commented Mar 15, 2017

SamVerschueren commented Apr 2, 2017

luca-aurelia commented Apr 3, 2017

SamVerschueren commented Apr 21, 2017

luca-aurelia commented Apr 25, 2017

sindresorhus commented Jun 26, 2017

luca-aurelia commented Jul 14, 2017

SamVerschueren 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

luca-aurelia commented Sep 28, 2017

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

sindresorhus commented Jun 10, 2019

luca-aurelia commented Mar 9, 2017 •

edited