Skip to content
This repository has been archived by the owner on Aug 8, 2024. It is now read-only.

Added support for object variables. Renamed withUUID to withActionOutput #14

Merged
merged 5 commits into from
Nov 28, 2018

Conversation

xAlien95
Copy link
Contributor

@xAlien95 xAlien95 commented Nov 24, 2018

Checks

  • I've checked there are no linting errors.
  • I've checked the code is still at 100% test coverage.

Added Actions (if relevant)

  • None

Are you happy to be listed as a contributor in the actions list here?

Yes

Any other information / comments

This pull request is the first of a multi step process that will let Shortcuts JS to handle both simple and extended variables, magic variables and global variables:

  1. Use of Attachment as base [Done]
  2. Use of WFSerialization as base
  3. Implementation of Aggrandizements for extended variables:
    an Attachment can have an Aggrandizements property used to define type coercion (e.g. use a variable as Text or Dictionary or File) and to access variable sub-properties (e.g. Dictionary Key, File Extension, Image Size)

This step is the only one with a breaking change on how the user has to write a Shortcut:

  • variables must be initialized with the variable() help function;
  • magic variables must be initialized with the actionOutput() help function.

Here's a comparison between the old and new methods:

// Old method

const {
  buildShortcut,
  withVariables
} = require('@joshfarrant/shortcuts-js');

const {
  calculate,
  number,
  showResult
} = require('@joshfarrant/shortcuts-js/actions');

let calcId;

const actions = [
  number({
    number: 42,
  }),
  calculate({
    operand: 3,
    operation: '/',
  }, (id) => {
    calcId = id;
  }),
  showResult({
    text: withVariables`Total is ${calcId}!`,
  }),
];
// New method

const {
  actionOutput,
  buildShortcut,
  withVariables,
} = require('@joshfarrant/shortcuts-js');

const {
  calculate,
  number,
  showResult
} = require('@joshfarrant/shortcuts-js/actions');

let calcVar = actionOutput('My Total');

const actions = [
  number({
    number: 42,
  }),
  calculate({
    operand: 3,
    operation: '/',
  }, calcVar),
  showResult({
    text: withVariables`Total is ${calcVar}!`,
  }),
];

Copy link
Owner

@joshfarrant joshfarrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, thank you so much for your hard work!

Just a few very small requests.

README.md Outdated Show resolved Hide resolved
__tests__/utils/index.spec.ts Outdated Show resolved Hide resolved
__tests__/utils/index.spec.ts Outdated Show resolved Hide resolved
__tests__/utils/withVariables.spec.ts Outdated Show resolved Hide resolved
@xAlien95
Copy link
Contributor Author

@joshfarrant, I split actionOutput, variable and I put variables in the main bundle as proposed here.

Commit 667e898 adds actions.js, is it needed? The src/actions folder already has an index.ts with all the exports inside. Am I missing something?

@joshfarrant
Copy link
Owner

Fantastic! I’ll look at merging this tomorrow.

For some reason trying to import actions from shortcuts-is/actions doesn’t work without the actions.js.

I’d have thought it would look for the actions file/directory relative to the module’s ‘main’ (the src directory) but that didn’t seem to work iirc.

I might well have missed something with that, so I’m open to input on it.

@xAlien95
Copy link
Contributor Author

xAlien95 commented Nov 27, 2018

For some reason trying to import actions from shortcuts-is/actions doesn’t work without the actions.js.

I need to add a variables.js in the main bundle then, similar to your actions.js file.
The same has to be done for a /meta subfolder (#15).

If you can make the node module to have the /build contents to be directly in @joshfarrant/shortcuts-js, it could be a little bit cleaner, since actions.ts, variables.ts and meta.ts could be put in /src in this repo:

node_modules/
  @joshfarrant/
    shortcuts-js/
      actions/...
      interfaces/...
      utils/...
      index.js
      index.d.ts
      variables.js
      variables.d.ts
      actions.js
      actions.d.ts
      README.md
      LICENSE
      package.json
      ...

@joshfarrant
Copy link
Owner

Looks great, thanks again for your work on this!

I'll take a look at building to the root of the project too 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants