Unexpected token delete with addon-actions

[silvenon]

I'm using the alpha version.

import { action } from '@storybook/addon-actions';

// ...later, in some story...

action('delete');

This results in the following error:

SyntaxError: Unexpected token delete

      at action (node_modules/@storybook/addon-actions/dist/preview.js:59:42)
      at Object.<anonymous> (web/static/js/sidebar/document.stories.js:19:38)
      at node_modules/@storybook/addon-storyshots/dist/require_context.js:39:24
      at Array.forEach (native)
      at requireModules (node_modules/@storybook/addon-storyshots/dist/require_context.js:34:9)
      at node_modules/@storybook/addon-storyshots/dist/require_context.js:48:7
      at Array.forEach (native)
      at requireModules (node_modules/@storybook/addon-storyshots/dist/require_context.js:34:9)
      at Function.newRequire.context (node_modules/@storybook/addon-storyshots/dist/require_context.js:90:5)
      at evalmachine.<anonymous>:23:21
      at runWithRequireContext (node_modules/@storybook/addon-storyshots/dist/require_context.js:102:3)
      at testStorySnapshots (node_modules/@storybook/addon-storyshots/dist/index.js:94:35)
      at Object.<anonymous> (web/static/js/storyshots.test.js:5:31)

[ndelangen]

Hey @silvenon
Unfortunately, you cannot use reserved words are action names, sorry. If you use 'my_delete' it should be fine.

Still we could maybe come up with a way of actually supporting this.. I'll mark this as a bug.

[silvenon]

Thanks! I have no problem changing this to a non-reserved word, it just took me a while to realize what's wrong, 😅 I wanted to create this issue so that other users figure this out more easily.

Thanks for all your hard work on Storybook! ❤️ 🍻

[gouegd]

I had the same issue, thanks for raising this and for the discussion ! 👍

I'll add that this is a new behaviour, as my stories with a delete action worked fine until recently.

[gouegd]

Digging a bit, it comes from the eval here.

At this point I don't see why getting the proper (as in ES6-proper) function name is important here, I tried changing the code to an unnamed function and the action log worked fine with my delete action. Maybe I'm missing something ?
Alternatively, we could add a prefix to the action name, such as action_, so the reserved keywords wouldn't be a problem.

[ndelangen]

Did you change, transpile and then run the code?

Reading the comments it sounds like there was a problem with the transpiled code, and this was a hack to solve that issue.

Good find!

[tmeasday]

I wonder what babel does that the comment is referring to. When I put

x = { delete: function() {} }

in here: https://babeljs.io

it produces the sensible:

x = { delete: function _delete() {} }

[gouegd]

@ndelangen no, I changed the transpiled code directly (in my node_modules folder) to experiment. I wanted to see if using an anonymous function would change anything to my actions logging in storybook - but it didn't.

@tmeasday I modified your sample to something slightly different and more similar to the code we run today:

const name = 'delete'
x = { [name]: function() {} }

The babel output then changes to something that uses Object.defineProperty, as in the code comments where I pointed previously.

[tmeasday]

Good point @gouegd!

After some hunting, I discovered the rationale: storybook-eol/storybook-addon-actions#24

[tmeasday]

I would suggest the tradeoff isn't worth it here! But it's hard to go back..

[gouegd]

In that case a prefix like _ could be a good middle-ground ?
The actions would still appear named in the debug tools (but with an extra prefix) and no crash would occur for names that are forbidden in the function declaration (like delete).

Another solution I can think of is to put the eval in a try/catch and then add the prefix only when necessary, but that gets quite ugly.

[tmeasday]

The prefix seems like a good move to me. Maybe @gnarf could weigh in, just in case we missed something?

[gnarf]

Maybe prefix it action_ ... The only reason to have this is for showing a reasonable name in the redux inspector

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 60 days. Thanks!

[ndelangen]

All suggested solutions sound great to me. Anyone want to pick this up and create a PR?

[Hypnosphi]

I think we can actually replace the eval with something like this:

Object.defineProperty(handler, 'name', { value: fnName });
return handler

This will probably allow to enable some transforms in #1733 which are currently disabled

[ndelangen]

[gouegd]

@Hypnosphi the eval was intentionally put there to avoid Object.defineProperty as you can see in the code comment. However I do not see the current workaround as particularly useful, as expressed here

[Hypnosphi]

Actually, the comment mentions a completely different defineProperty.
The author's idea was that when processing {[fnName]: function() {}}, JS engines automatically guess the function's "name" as the value of fnName:

const fnName = 'foo'
const obj = {[fnName]: function() {}}
console.log(obj[fnName].name) // logs "foo"

But babel transpiles it to following:

'use strict';

function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }

var fnName = 'foo';
var obj = _defineProperty({}, fnName, function () {});
console.log(obj[fnName].name); // logs "undefined"

Here, the fnName property is assigned after the object (the empty one) is created. That's why the function doesn't get any name.

You can see that defineProperty here assigns the obj[fnName] field, while my suggestion is to assign handler.name field. It cannot be changed with plain handler.name = fnName assignment, only with Object.defineProperty.

As for the question why the name is needed at all, the main benefit is that it's visible in error stacktraces which simplifies debugging.

[gouegd]

Well, when I said it was "put there to avoid Object.defineProperty", I can hardly see it as a completely different thing than what the comment mentioned as "but babel transpiles to Object.defineProperty"

Anyway, good points otherwise 👍

[Hypnosphi]

My point is that the Object.defineProperty I introduced here is not the one that comment's author wanted to avoid as a result of transpilation. It applies to a different object, and sets a different property

[Hypnosphi]

Fixed in 3.2.16