Skip to content

[codemod][prereq] Convert Functions from arrow to function - #35749

Merged
clintandrewhall merged 1 commit into
elastic:masterfrom
clintandrewhall:codemod-functions
Apr 29, 2019
Merged

clintandrewhall merged 1 commit into
elastic:masterfrom
clintandrewhall:codemod-functions

Conversation

@clintandrewhall

@clintandrewhall clintandrewhall commented Apr 29, 2019 •

Copy link
Copy Markdown
Contributor

Summary

This is a codemod to support #35087. Including this change in #35087 makes the differences too extreme to review effectively, (git thinks the files are all new because of the spacing indents, etc). By pushing this change to master first, we can better review the changes there.

This PR was created by running a RegEx to convert the Function arrow expressions and EOF to match.

Why is this change necessary?

There's a bug in Typescript that will affect how our Function files appear and the features they provide: microsoft/TypeScript#241

It's called "return type widening"... here's a practical example of how a typed file looks at the moment:

import { FunctionFactory } from '../types';

interface Arguments {
  value: string[];
}

export const string: FunctionFactory<'string', Arguments, string> = () => ({
  name: 'string',
  aliases: [],  // <- THIS IS NOT IN THE FunctionSpec TYPE
  type: 'string',
  help:
    'Output a string made of other strings. Mostly useful when combined with sub-expressions that output a string, ' +
    ' or something castable to a string',
  args: {
    value: {
      aliases: ['_'],
      types: ['string'],
      multi: true,
      help: "One or more strings to join together. Don't forget spaces where needed!",
    },
  },
  fn: (_context, args) => args.value.join(''),
});

Since the const does not have an inline return type, the return type from FunctionFactory is "widened" to accept anything so long as the rest of the returned object matches the type.

As a result, you don't get an error if you include something that shouldn't be there, or isn't documented in the type.

There are a few options to fix this, but each affects how the file is constructed.

Option One: we include the return type inline.

It's long, repetitive, and ugly.

export const string: FunctionFactory<'string', Arguments, string> = (): FunctionSpec<
  'string',
  Arguments,
  string
> => ({
  name: 'string',
  aliases: [],  // <- THIS IS NOT DOCUMENTED, THROWS TYPE ERROR

Option Two: we convert the const to a pure function.

export function string(): FunctionSpec<'string', Arguments, string> {
  return {
    name: 'string',
    aliases: [],  // <- THIS IS NOT DOCUMENTED, THROWS TYPE ERROR
    ...
  };
}

Option Three: we wrap the spec in a strongly-typed function

We use the Neverize technique.

So I went with 2

I chatted with @w33ble and @rashidkpc ... this seemed to be the least invasive with the tersest syntax.

@clintandrewhall clintandrewhall added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.0.0 v7.2.0 labels Apr 29, 2019
@clintandrewhall
clintandrewhall requested a review from a team as a code owner April 29, 2019 16:18
@elasticmachine

Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-canvas

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@rashidkpc

rashidkpc commented Apr 29, 2019 •

Copy link
Copy Markdown
Contributor

We talked about this ahead of the pull, I didn't run this, but the changes are all formatting so LGTM.

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

Labels

Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.2.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants