Addon-actions: Fix types and refactor #2438

rhalff · 2017-12-05T23:25:30Z

Primitive types failed to be logged.

Refactor of the code, split functionality into different files and added tests.

I've added an extra example in the kitchen sink app which logs different types.

The changes from #2401 were merged.

…' into fix-types-and-refactor

codecov · 2017-12-05T23:44:52Z

Codecov Report

Merging #2438 into release/3.3 will increase coverage by 0.39%.
The diff coverage is 49.14%.

@@               Coverage Diff               @@
##           release/3.3    #2438      +/-   ##
===============================================
+ Coverage        19.11%   19.51%   +0.39%     
===============================================
  Files              355      386      +31     
  Lines             8259     8707     +448     
  Branches           901      959      +58     
===============================================
+ Hits              1579     1699     +120     
- Misses            6003     6263     +260     
- Partials           677      745      +68

Impacted Files	Coverage Δ
addons/actions/src/lib/errors/index.js	`0% <ø> (ø)`
app/react/src/client/preview/render.js	`0% <ø> (ø)`	⬆️
...ddons/actions/src/containers/ActionLogger/index.js	`2.98% <0%> (+0.08%)`	⬆️
addons/actions/src/lib/util/index.js	`0% <0%> (ø)`
app/react-native/src/bin/storybook-start.js	`0% <0%> (ø)`	⬆️
addons/actions/src/lib/errors/DecycleError.js	`0% <0%> (ø)`
...actions/src/lib/types/function/reservedKeywords.js	`25% <25%> (ø)`
.../actions/src/lib/types/object/canAccessProperty.js	`30% <37.5%> (ø)`
...tions/src/lib/types/function/createFunctionEval.js	`22.22% <40%> (ø)`
addons/actions/src/lib/retrocycle.js	`34.09% <40.54%> (ø)`
... and 113 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bad51f5...ddb21db. Read the comment docs.

Hypnosphi · 2017-12-05T23:46:33Z

addons/actions/src/lib/util/getPropertiesList.js

+const { hasOwnProperty } = Object.prototype;
+
+export default function getPropertiesList(value) {
+  const keys = Object.getOwnPropertyNames(value);


This includes non-enumerable props. I don't think they should be displayed. Just for ... in should be enough

I tried to get the same behavior as in the console for something like the Math object, which does show the non-enumerable properties, otherwise it would just be empty.

console shows them in half-transparent style, to indicate that they're somewhat secondary. And it logs __proto__ among them.

And I can't actually imagine someone using Math object as event handler attribute. Let's not add non-enumerable props unless we find a usecase.

Hypnosphi · 2017-12-06T10:41:23Z

Live example

Hypnosphi · 2017-12-06T10:53:49Z

addons/actions/src/lib/types/function/isReserved.js

@@ -0,0 +1,3 @@
+export default function isReserved(name) {
+  return ['delete'].indexOf(name) >= 0;


There's a lot of other reserved keywords: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Keywords

You can also consider this approach instead of appending $:
https://github.com/rhalff/storybook/blob/master/addons/actions/src/preview.js#L27-L33

Tried to solve it by supporting both methods, preferring the one you have suggests.
The eval version could be removed if you find it unnecessary.

I've included the changes of #2428 but moved the check to a different location.

Hypnosphi · 2017-12-06T10:58:22Z

addons/actions/src/lib/types/class/createFakeConstructor.js

+    Object.assign(this, data);
+  }
+
+  Object.defineProperty(FakeConstructor, 'name', {


This will break in android emulator, see #2324

Adding canConfigureName check should solve this

This should also be fixed now, I didn't test it on android though.

Hypnosphi · 2017-12-06T11:02:11Z

addons/actions/src/lib/util/__tests__/getPropertiesList.test.js

@@ -0,0 +1,62 @@
+/* global File */


require { File, Math } from 'global'

This reverts commit 9bda5ab.

Hypnosphi · 2017-12-07T16:45:50Z

addons/actions/src/lib/util/getPropertiesList.js

+  for (const name in value) {
+    // noinspection JSUnfilteredForInLoop
+    if (
+      keys.indexOf(name) === -1 &&


Why is this check needed? There's no chance that for .. in yields duplicate keys:

const a = {foo: 'foo', bar: 'bar'} const b = Object.create(a, {foo: {value: 'baz', enumerable: 'true'}}) for (key in b) { console.log(key) } VM372:1 foo VM372:1 bar

Note that foo is logged only once

Sorry, a left over from populating the keys array with ownPropertyNames first, I will remove it.

The function check is needed to only show functions in case they are own properties of the current object.

The previous fix only worked because the results were stringified, filtering out all functions e.g. for .. in over a file object does include slice etc. but were not shown, because of the stringification later on.

Hypnosphi · 2017-12-07T16:47:49Z

addons/actions/src/lib/util/getPropertiesList.js

+    // noinspection JSUnfilteredForInLoop
+    if (
+      keys.indexOf(name) === -1 &&
+      !(typeof value[name] === 'function' && !hasOwnProperty.call(value, name))


Let's invert the check, so that it's more clear what it does (picks all the own properties, and non-function delegated properties):
if (hasOwnProperty.call(value, name)) || typeof value[name] !== 'function')

Hypnosphi · 2017-12-11T20:07:41Z

@rhalff can you please extract the fix for primitive types into a separate PR? We need it for 3.3.0 release

rhalff · 2017-12-12T19:57:21Z

I do not think that will be a better merge than the one currently offered. The main problems I found in the current state of the pull request is Firefox not liking some objects as weakmap keys and IE Edge and down still consider some decycled objects as cyclic. This only happens on logging the 'window' object though. Not at the level of e.g SyntheticEvent. I would prefer updating this pull request and just return the errors to the action logger.

Hypnosphi · 2017-12-12T21:55:35Z

OK, let's do that

…d-refactor

…-types-and-refactor

rhalff · 2017-12-13T18:42:17Z

@Hypnosphi I've added the error handling, to trigger some errors you can cllick the window type button in firefox and IE / Edge, perhaps there are better solutions to solve this, but it works for now.

Hypnosphi · 2017-12-13T18:47:11Z

Great. Please run yarn locally to update lockfile

Hypnosphi · 2017-12-13T19:20:21Z

Please also update storyshots: yarn test --core --update

~~The CLI test failure is probably caused by some vue upgrade, I'll try to handle it~~ done

…-types-and-refactor

rhalff · 2017-12-14T11:03:48Z

Storyshots is also updated now, I still get a warning when I run jest though:

$ ./scripts/test.js --core --update
jest-haste-map: duplicate manual mock found:
  Module name: example
  Duplicate Mock path: /home/rhalff/git/storybook/addons/actions/dist/lib/__mocks__/example.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in: 
/home/rhalff/git/storybook/addons/actions/dist/lib/__mocks__/example.js
 Please delete one of the following two files: 
 /home/rhalff/git/storybook/addons/actions/src/lib/__mocks__/example.js
/home/rhalff/git/storybook/addons/actions/dist/lib/__mocks__/example.js

Should the babel ignore pattern be updated to also ignore the __mocks__ folders?

I think it's defined in scripts/prepare.js

Hypnosphi · 2017-12-14T11:06:04Z

Should the babel ignore pattern be updated to also ignore the mocks folders?

Yes, I think so. Please adjust prepare.js

…-types-and-refactor

ndelangen · 2017-12-14T17:43:38Z

@rhalff Thank you for this epic PR, I know you spend a lot of work and time on this 🙇

Are you planning to open more PR's? I can add you to the github organisation to make this easier if you want?

rhalff · 2017-12-14T19:49:46Z

@ndelangen Thanks! I'm not planning more PR's at the moment though. Besides that the current method is easy enough. @Hypnosphi thanks for reviewing.

Hypnosphi · 2017-12-14T20:12:53Z

is easy enough

Actually, when the PR branch is in the same repo, it makes things a bit easier for reviewers =)

rhalff · 2017-12-15T00:45:47Z

In that case, add me.. :)

Hypnosphi and others added 7 commits November 30, 2017 01:57

Addon-actions: display enumerable properties from prototype chain

ad89dca

Merge remote-tracking branch 'origin/action-list-all-enumerable-props…

84244f3

…' into fix-types-and-refactor

refactor code to better support all types

24c0868

add global dependency

611a021

add buttons to test each type individually

af94539

moved to All types

440b333

separate tests

1c3ed2d

rhalff mentioned this pull request Dec 5, 2017

action-logger addon don't show Array<File> options #1945

Closed

Hypnosphi requested changes Dec 5, 2017

View reviewed changes

Hypnosphi mentioned this pull request Dec 5, 2017

Addon-actions: display enumerable properties from prototype chain #2401

Closed

Hypnosphi added this to the v3.3.0 milestone Dec 5, 2017

Hypnosphi added addon: actions bug labels Dec 5, 2017

Hypnosphi requested review from ndelangen and shilman December 5, 2017 23:58

Hypnosphi requested changes Dec 6, 2017

View reviewed changes

rhalff and others added 7 commits December 6, 2017 13:15

import from global instead of using eslint

a398583

do not list non-enumerable properties

9bda5ab

Revert "do not list non-enumerable properties"

9a46c6e

This reverts commit 9bda5ab.

do not list non-enumerable properties & ignore non own functions

9f577c3

fix typos & rename some values

238367b

include direct action call in examples

4279540

Merge branch 'release/3.3' into fix-types-and-refactor

6403dd3

Hypnosphi reviewed Dec 7, 2017

View reviewed changes

Hypnosphi requested changes Dec 7, 2017

View reviewed changes

rhalff added 2 commits December 7, 2017 17:05

prefer configure to eval

84f3343

update storyshot

442df6e

danielduan added this to the v3.4.0 milestone Dec 11, 2017

Hypnosphi modified the milestones: v3.4.0, v3.3.0 Dec 12, 2017

Merge branch 'release/3.3' into fix-types-and-refactor

0ce1ad2

rhalff added 3 commits December 13, 2017 18:21

add error handling

a35d8e4

Merge remote-tracking branch 'upstream/release/3.3' into fix-types-an…

d693516

…d-refactor

Merge remote-tracking branch 'origin/fix-types-and-refactor' into fix…

9d9a728

…-types-and-refactor

Hypnosphi approved these changes Dec 13, 2017

View reviewed changes

Update lockfile

1a10635

Merge branch 'release/3.3' into fix-types-and-refactor

a9022b5

tmeasday mentioned this pull request Dec 14, 2017

[Release 3.3]: Cannot log non-object arguments in actions #2481

Closed

rhalff and others added 3 commits December 14, 2017 10:44

update storyshots

8aba7c8

Merge remote-tracking branch 'origin/fix-types-and-refactor' into fix…

7dab15f

…-types-and-refactor

Merge branch 'release/3.3' into fix-types-and-refactor

b08bc28

rhalff added 2 commits December 14, 2017 11:16

ignore __mocks__/ folders

d35e696

Merge remote-tracking branch 'origin/fix-types-and-refactor' into fix…

ddb21db

…-types-and-refactor

Hypnosphi removed the in progress label Dec 14, 2017

Hypnosphi merged commit f4cbe6f into storybookjs:release/3.3 Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addon-actions: Fix types and refactor #2438

Addon-actions: Fix types and refactor #2438

rhalff commented Dec 5, 2017

codecov bot commented Dec 5, 2017 •

edited

Loading

Hypnosphi Dec 5, 2017

rhalff Dec 6, 2017

Hypnosphi Dec 6, 2017 •

edited

Loading

Hypnosphi commented Dec 6, 2017

Hypnosphi Dec 6, 2017

rhalff Dec 7, 2017

Hypnosphi Dec 6, 2017

Hypnosphi Dec 7, 2017

rhalff Dec 7, 2017

Hypnosphi Dec 6, 2017

Hypnosphi Dec 7, 2017 •

edited

Loading

rhalff Dec 7, 2017

Hypnosphi Dec 7, 2017 •

edited

Loading

Hypnosphi commented Dec 11, 2017 •

edited

Loading

rhalff commented Dec 12, 2017

Hypnosphi commented Dec 12, 2017

rhalff commented Dec 13, 2017

Hypnosphi commented Dec 13, 2017

Hypnosphi commented Dec 13, 2017 •

edited

Loading

rhalff commented Dec 14, 2017

Hypnosphi commented Dec 14, 2017

ndelangen commented Dec 14, 2017

rhalff commented Dec 14, 2017

Hypnosphi commented Dec 14, 2017

rhalff commented Dec 15, 2017

		@@ -0,0 +1,3 @@
		export default function isReserved(name) {
		return ['delete'].indexOf(name) >= 0;

Addon-actions: Fix types and refactor #2438

Addon-actions: Fix types and refactor #2438

Conversation

rhalff commented Dec 5, 2017

codecov bot commented Dec 5, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Hypnosphi commented Dec 6, 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

Hypnosphi Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Hypnosphi commented Dec 11, 2017 • edited Loading

rhalff commented Dec 12, 2017

Hypnosphi commented Dec 12, 2017

rhalff commented Dec 13, 2017

Hypnosphi commented Dec 13, 2017

Hypnosphi commented Dec 13, 2017 • edited Loading

rhalff commented Dec 14, 2017

Hypnosphi commented Dec 14, 2017

ndelangen commented Dec 14, 2017

rhalff commented Dec 14, 2017

Hypnosphi commented Dec 14, 2017

rhalff commented Dec 15, 2017

codecov bot commented Dec 5, 2017 •

edited

Loading

Hypnosphi Dec 6, 2017 •

edited

Loading

Hypnosphi Dec 7, 2017 •

edited

Loading

Hypnosphi Dec 7, 2017 •

edited

Loading

Hypnosphi commented Dec 11, 2017 •

edited

Loading

Hypnosphi commented Dec 13, 2017 •

edited

Loading