Export core events constants as named exports

[dandean]

Issue: #5130 #5140

What I did

The PR #5140 introduced a regression as noted by @tmeasday here #5140 (comment)

This looks like the result of incompatibilities between the module systems where module.exports = { ... } would provide both a default export and named exports associated with the keys of the exported object.

That PR broke that duality by only exporting the enum as the default export and not also exporting named exports.

This PR declares named exports associated with each of the keys of the events enum.

How to test

• Write tests which prove that default imports and named imports both work.

[dandean]

The solution here provides backwards compatibility with what the CommonJS module was providing but it's a bit bulky and kinda weird.

If I were to choose a cleaner solution it would be to not export the enum and instead only have named exports. This would allow consumers to do either of these:

import * as Events from '@storybook/core-events';
import { FOO } from '@storybook/core-events';

The downside is that it is a breaking change as this no longer works:

import Events from '@storybook/core-events';

The implication of going with just named exports is that this patch would also need to include changes to the rest of this repository to use either named imports or * as Events.

Public non-@storybook/ modules which depend on this module and may be impacted by the breaking change are listed by npm:

• @wesrice/addon-a11y
• @beisen/storybook-core
• addon-knobs-null-number-fix
• @brycemhammond/addon-knobs
• @beisen/storybook-ui
• @beisen/storybook-addon-viewport

[dandean]

One more perspective on this.

I just did a search through the this repository's next branch and there's only one instance of the module being imported via destructing. All other imports use the default import.

If picking a single import pattern to support is desired then exporting the enum as the default export and not exporting named exports would be the least invasive.

[tmeasday]

I just did a search through the this repository's next branch and there's only one instance of the module being imported via destructing. All other imports use the default import.

That's interesting. On the tech/overhaul-ui branch there are about 50/50 uses of both. It looks like @ndelangen was switching to the destructuring syntax (e.g. https://github.com/storybooks/storybook/pull/5148/files/07770e1c9eaac2a097b2f43d0b091d89188ff4b4..146f0d6643d12fb4832f06f49a1c755fc3823d99). I guess I'd be interested in his thoughts here.

I think given that it's safe to assume that most external uses will be via the default export.

[dandean]

I think it's probably best to maintain compatibility for 4.x. The Storybook 5.0 release would be a good time to assert a single export format.

I've added a test file to ensure that all previously supported import variants are still supported.

[codecov]

Codecov Report

Merging #5186 into next will increase coverage by 0.39%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             next    #5186      +/-   ##
==========================================
+ Coverage   29.85%   30.24%   +0.39%     
==========================================
  Files         613      613              
  Lines        8883     8887       +4     
  Branches     1217     1217              
==========================================
+ Hits         2652     2688      +36     
+ Misses       5558     5527      -31     
+ Partials      673      672       -1

Impacted Files	Coverage Δ
lib/core-events/src/index.ts	100% <100%> (+100%)	⬆️
lib/theming/src/themes/light-syntax.ts
lib/theming/src/themes/dark-syntax.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 9d14ec6...0579f64. Read the comment docs.

[tmeasday]

I cherry picked b870091 into release/5.0 as this was causing serious problems there. If we don't end up merging this PR we should probably undo that

[dandean]

Let me know if you need anything else from me on this.

[tmeasday]

@dandean no this looks really great, thank you for this. I will just wait to hear from @ndelangen before deciding what to do.

We are looking at merging 5.0 into next in the near future by the way, which makes the question of "what to do pre-5.0" a little academic ;)

[ndelangen]

I think this means by now this change is already on next?

[tmeasday]

@ndelangen I cherry-picked a single commit from this branch. We should probably merge the entire branch as it includes tests etc.

[shilman]

@dandean @ndelangen @tmeasday Removing the patch label. AFAIK we use that to denote PRs that need to be cherry-picked between next and master. LMK if I got that wrong. 🙇