Migrate @storybook/core-events to TypeScript #5140

dandean · 2019-01-04T00:07:59Z

What I did

Migrated @storybook/core-events to TypeScript.

Many packages depend on @storybook/core-events. Before migrating those packages to TypeScript it might make sense to migrate this one first. Also, this is my first contribution to this project and doing something small to get used to your process seems like the right way to go.

How to test

I don't believe there is anything to test in this work. Please let me know if you'd like to see anything here.

cc @kroeder

dandean · 2019-01-04T00:35:53Z

It looks like @storybook/ui can't find these changes. Any pointers on what's going wrong here?

kroeder · 2019-01-04T00:56:34Z

I will check this later today

ndelangen · 2019-01-04T11:01:07Z

So awesome to have you helping us @dandean! 👏

kroeder · 2019-01-04T13:41:14Z

lib/core-events/src/index.ts

+  STORY_THREW_EXCEPTION: string;
+}
+
+const events: CoreEvents = {


Do you think we can throw away the above interface if we use enum ?
Both solutions offer auto-completion in js and ts

I did

enum events { CHANNEL_CREATED = 'channelCreated', GET_CURRENT_STORY = 'getCurrentStory', SET_CURRENT_STORY = 'setCurrentStory', GET_STORIES = 'getStories', SET_STORIES = 'setStories', SELECT_STORY = 'selectStory', APPLY_SHORTCUT = 'applyShortcut', STORY_ADDED = 'storyAdded', FORCE_RE_RENDER = 'forceReRender', REGISTER_SUBSCRIPTION = 'registerSubscription', STORY_RENDERED = 'storyRendered', STORY_ERRORED = 'storyErrored', STORY_THREW_EXCEPTION = 'storyThrewException', } export default events;

and it compiled to

Object.defineProperty(exports, "__esModule", { value: true }); var events; (function (events) { events["CHANNEL_CREATED"] = "channelCreated"; events["GET_CURRENT_STORY"] = "getCurrentStory"; events["SET_CURRENT_STORY"] = "setCurrentStory"; events["GET_STORIES"] = "getStories"; events["SET_STORIES"] = "setStories"; events["SELECT_STORY"] = "selectStory"; events["APPLY_SHORTCUT"] = "applyShortcut"; events["STORY_ADDED"] = "storyAdded"; events["FORCE_RE_RENDER"] = "forceReRender"; events["REGISTER_SUBSCRIPTION"] = "registerSubscription"; events["STORY_RENDERED"] = "storyRendered"; events["STORY_ERRORED"] = "storyErrored"; events["STORY_THREW_EXCEPTION"] = "storyThrewException"; })(events || (events = {})); exports.default = events;

before it was

var events = { CHANNEL_CREATED: 'channelCreated', GET_CURRENT_STORY: 'getCurrentStory', SET_CURRENT_STORY: 'setCurrentStory', GET_STORIES: 'getStories', SET_STORIES: 'setStories', SELECT_STORY: 'selectStory', APPLY_SHORTCUT: 'applyShortcut', STORY_ADDED: 'storyAdded', FORCE_RE_RENDER: 'forceReRender', REGISTER_SUBSCRIPTION: 'registerSubscription', STORY_RENDERED: 'storyRendered', STORY_ERRORED: 'storyErrored', STORY_THREW_EXCEPTION: 'storyThrewException', }; exports.default = events;

Works for me – I'll give that change a shot.

kroeder · 2019-01-04T13:52:00Z

You can fix the yarn.lock conflict by just running yarn locally and push the changed file

yarn bootstrap --core works for me locally..
@ndelangen any idea why it is failing in CI?

ndelangen · 2019-01-04T19:58:01Z

error in CI:

ERROR in dll storybook_ui
Module not found: Error: Can't resolve '@storybook/core-events' in '/tmp/storybook/lib/ui'

I get the same locally:

lerna ERR! yarn run createDlls stderr:
failed:  Hash: 2ccc140332d5be684de3
Version: webpack 4.27.1
Time: 15314ms
Built at: 2019-01-04 21:05:32
                   Asset      Size  Chunks  Chunk Names
storybook_ui_dll.LICENCE  2.89 KiB
     storybook_ui_dll.js   730 KiB       0  storybook_ui
Entrypoint storybook_ui = storybook_ui_dll.js
[0] dll storybook_ui 12 bytes {0} [built]
[02Wq] ../components/dist/icons/index.js 411 bytes {0} [built]
[3yYM] /Users/dev/Projects/GitHub/storybook/core/node_modules/regenerator-runtime/runtime.js 23 KiB {0} [built]
[7nmT] /Users/dev/Projects/GitHub/storybook/core/node_modules/react-dom/index.js 1.33 KiB {0} [built]
[ARua] /Users/dev/Projects/GitHub/storybook/core/node_modules/airbnb-js-shims/index.js 40 bytes {0} [built]
[ERkP] /Users/dev/Projects/GitHub/storybook/core/node_modules/react/index.js 190 bytes {0} [built]
[KTRZ] ../addons/dist/public_api.js 644 bytes {0} [built]
[SbT1] ./dist/index.js 3.52 KiB {0} [built]
[aQcq] /Users/dev/Projects/GitHub/storybook/core/node_modules/core-js/fn/array/iterator.js 107 bytes {0} [built]
[aWzz] /Users/dev/Projects/GitHub/storybook/core/node_modules/prop-types/index.js 956 bytes {0} [built]
[adtJ] ../components/dist/index.js 4.06 KiB {0} [built]
[l1C2] /Users/dev/Projects/GitHub/storybook/core/node_modules/@emotion/core/dist/core.browser.esm.js 5.16 KiB {0} [built]
[tCgp] /Users/dev/Projects/GitHub/storybook/core/node_modules/@emotion/provider/dist/provider.esm.js 1.65 KiB {0} [built]
[x6gb] /Users/dev/Projects/GitHub/storybook/core/node_modules/@emotion/styled/dist/styled.esm.js 1.33 KiB {0} [built]
[yXcf] /Users/dev/Projects/GitHub/storybook/core/node_modules/core-js/es6/symbol.js 131 bytes {0} [built]
    + 705 hidden modules

ERROR in dll storybook_ui
Module not found: Error: Can't resolve '@storybook/core-events' in '/Users/dev/Projects/GitHub/storybook/core/lib/ui'
 @ dll storybook_ui storybook_ui[5]

ERROR in ./dist/libs/key_events.js
Module not found: Error: Can't resolve '@storybook/core-events' in '/Users/dev/Projects/GitHub/storybook/core/lib/ui/dist/libs'
 @ ./dist/libs/key_events.js 15:41-74
 @ ./dist/modules/ui/configs/handle_keyevents.js
 @ ./dist/modules/ui/index.js
 @ ./dist/index.js
 @ dll storybook_ui
error Command failed with exit code 1.

lerna ERR! yarn run createDlls exited 1 in '@storybook/ui'

On my local machine, no dist-folder is generated for lib/core-events

ndelangen · 2019-01-04T20:01:18Z

@dandean I invited you to the GitHub organisation,
You should now be able to push to branches on this repo, no need for forking around ;)

# Conflicts: # yarn.lock

ndelangen · 2019-01-04T20:10:52Z

found a FIX I think: the prepare script was missing in package.json.

This allows the DLL to build, let's see if another problem appears

dandean · 2019-01-04T20:14:04Z

Wonderful – thank you @ndelangen. I've got the change requests from @kroeder on my TODO list today so if this builds as expected now I'll get those patches in.

codecov · 2019-01-04T21:37:09Z

Codecov Report

Merging #5140 into next will decrease coverage by 0.08%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##            next    #5140      +/-   ##
=========================================
- Coverage   35.3%   35.22%   -0.09%     
=========================================
  Files        596      596              
  Lines       7378     7392      +14     
  Branches    1014     1010       -4     
=========================================
- Hits        2605     2604       -1     
- Misses      4261     4275      +14     
- Partials     512      513       +1

Impacted Files	Coverage Δ
lib/core-events/src/index.ts	`0% <0%> (ø)`

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 5108da8...008ae8a. Read the comment docs.

ndelangen · 2019-01-05T08:57:46Z

Amazing work @dandean ! 👏

dandean · 2019-01-05T18:33:35Z

Thanks for your help, @kroeder and @ndelangen.

tmeasday · 2019-01-08T11:34:40Z

I'm not sure I fully understand the subtlety but I want to flag that I had to make this change: 5cfd3c8

And it looks like a lot of other files use the events in similar ways. I'm not sure what needs to change but hopefully someone that has more time and context can fill me in please?

dandean · 2019-01-08T17:44:14Z

@tmeasday Thanks for noting this. I think I understand this issue, and that it's a conflict between how the module systems operate.

In the CommonJS module system (depending on how babel is configured) using module.exports = { ... }, the export is both the default export (enabling import Events from '@storybook/core-events') and a destructurable object (enabling import { CHANNEL_CREATED } from '@storybook/core-events').

That duality isn't automatically supported in the ES6 and TypeScript module systems, so I think we need to make it explicit by both exporting the enum as the default export and exporting each constant as a named export.

I will put a PR together.

dandean · 2019-01-08T17:52:00Z

WIP patch here: #5186

Migrate core-events to TypeScript

cb49bec

kroeder mentioned this pull request Jan 4, 2019

TypeScript migration organization #5030

Closed

kroeder reviewed Jan 4, 2019

View reviewed changes

ndelangen assigned dandean, ndelangen and kroeder Jan 4, 2019

ndelangen added core typescript labels Jan 4, 2019

ndelangen added this to the next milestone Jan 4, 2019

ndelangen added 2 commits January 4, 2019 21:06

Merge branch 'next' into pr/dandean/5140

1148480

# Conflicts: # yarn.lock

FIX missing prepare step for lib/core-events

79c4155

ndelangen added the maintenance User-facing maintenance tasks label Jan 4, 2019

Use an enum instead of an interface for core events

008ae8a

kroeder approved these changes Jan 5, 2019

View reviewed changes

ndelangen merged commit 58b755c into storybookjs:next Jan 5, 2019

dandean mentioned this pull request Jan 8, 2019

Export core events constants as named exports #5186

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate @storybook/core-events to TypeScript #5140

Migrate @storybook/core-events to TypeScript #5140

dandean commented Jan 4, 2019

dandean commented Jan 4, 2019

kroeder commented Jan 4, 2019

ndelangen commented Jan 4, 2019

kroeder Jan 4, 2019

dandean Jan 4, 2019

kroeder commented Jan 4, 2019

ndelangen commented Jan 4, 2019 •

edited

Loading

ndelangen commented Jan 4, 2019

ndelangen commented Jan 4, 2019 •

edited

Loading

dandean commented Jan 4, 2019

codecov bot commented Jan 4, 2019 •

edited

Loading

ndelangen commented Jan 5, 2019

dandean commented Jan 5, 2019

tmeasday commented Jan 8, 2019

dandean commented Jan 8, 2019 •

edited

Loading

dandean commented Jan 8, 2019

Migrate @storybook/core-events to TypeScript #5140

Migrate @storybook/core-events to TypeScript #5140

Conversation

dandean commented Jan 4, 2019

What I did

How to test

dandean commented Jan 4, 2019

kroeder commented Jan 4, 2019

ndelangen commented Jan 4, 2019

kroeder Jan 4, 2019

Choose a reason for hiding this comment

dandean Jan 4, 2019

Choose a reason for hiding this comment

kroeder commented Jan 4, 2019

ndelangen commented Jan 4, 2019 • edited Loading

ndelangen commented Jan 4, 2019

ndelangen commented Jan 4, 2019 • edited Loading

dandean commented Jan 4, 2019

codecov bot commented Jan 4, 2019 • edited Loading

Codecov Report

ndelangen commented Jan 5, 2019

dandean commented Jan 5, 2019

tmeasday commented Jan 8, 2019

dandean commented Jan 8, 2019 • edited Loading

dandean commented Jan 8, 2019

ndelangen commented Jan 4, 2019 •

edited

Loading

ndelangen commented Jan 4, 2019 •

edited

Loading

codecov bot commented Jan 4, 2019 •

edited

Loading

dandean commented Jan 8, 2019 •

edited

Loading