Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Index: Support { csfData as default } CSF exports #18588

Merged

Conversation

Kingdutch
Copy link
Contributor

@Kingdutch Kingdutch commented Jun 28, 2022

Issue:

What I did

Parse csfMetadata from { someConst as default } exports.

This ensures that if CSF data is exported with the { data as default }
syntax rather than as export default data it is still recognized by
Storybook.

This improves interop with automated code-generation performed by the
ReScript compiler.

ReScript background

The kind people in Discord asked me to provide a bit of background on ReScript. ReScript is "Fast, Simple, Fully Typed JavaScript from the Future" and provides "a robustly typed language that compiles to efficient and human-readable JavaScript. It comes with a lightning fast compiler toolchain that scales to any codebase size."

Contrary to TypeScript, which tries to type all the intricacies of JavaScript, ReScript takes a sound typesystem (borrowed from OCaml) and compiles to valid, human-readable JavaScript.

With respect to this PR it's important to note that the proposed change improves interoperability with JavaScript in general and doesn't attempt to support some kind of non-JavaScript dialect.

export default csfData vs export { csfData as default }

Before submitting this PR I dove into the differences between export default and export { X as default }. Jake Archibald, developer advocate for Google Chrome, did an excellent deep dive. I'll highlight (what I believe to be) the most important bits for this PR below.

Imports are references, not values

In most cases (except destructuring of a dynamic import) what is imported is a live binding: changing the value in the exporting file changes the value in the importing file.

But 'export default' works differently

Because export default syntax supports exporting a value directly (e.g. export default "Foo") which is not supported in other exports (i.e. you can't export { "Foo" as X }) some special behavior is needed.

This means the interpreter sees export default <expression> as exporting the result of some expression. To make this work the result of the expression is assigned to a hidden variable which is actually exported.

And 'export { thing as default }' is different

Because export { thing as default } follows the "normal exporting behavior" and is not in the special export default form, it exports thing as a live-binding.

What this means for Storybook

I can generate three examples to illustrate what happens.

// exportDefault.mjs
// This is how it's recommended in the docs.
// We can't change the value of `title` here.
export default {
  title: "export default"
};
// exportDefaultObject.mjs
// This is currently supported.
const csfData = {
  title: "export default object"
};

export default csfData

// We could now change the title after import.
setTimeout(function () {
  csfData.title = "export default object changed";
}, 100);
// exportAsDefault.mjs
// This is not yet supported.
const csfData = {
  title: "export as default"
};

export { csfData as default };

// We could now also change the title after export
// but this is not new behavior.
setTimeout(function () {
  csfData.title = "export as default changed";
}, 100);
// import.mjs
import exportAsDefault from "./exportAsDefault.mjs"
import exportDefault from "./exportDefault.mjs"
import exportDefaultObject from "./exportDefaultObject.mjs"

console.log("Iniital");
console.log("exportDefault", exportDefault);
console.log("exportDefaultObject", exportDefaultObject);
console.log("exportAsDefault", exportAsDefault);

setTimeout(function () {
  console.log("Changed");
  console.log("exportDefault", exportDefault);
  console.log("exportDefaultObject", exportDefaultObject);
  console.log("exportAsDefault", exportAsDefault);
}, 200);
$ node import.mjs 
Iniital
exportDefault { title: 'export default' }
exportDefaultObject { title: 'export default object' }
exportAsDefault { title: 'export as default' }
Changed
exportDefault { title: 'export default' }
exportDefaultObject { title: 'export default object changed' }
exportAsDefault { title: 'export as default changed' }

As can be seen from Storybook's perspective (import.mjs) the two different forms are indistinguishable.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

This ensures that if CSF data is exported with the `{ data as default }`
syntax rather than as `export default data` it is still recognized by
Storybook.

This improves interop with automated code-generation performed by the
ReScript compiler.
@nx-cloud
Copy link

nx-cloud bot commented Jun 28, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 5e98e9a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@shilman shilman changed the title Allow { csfData as default } exports Index: Support { csfData as default } CSF exports Jul 11, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are looking good @Kingdutch . Will merge when CI passes 💯

@shilman shilman self-assigned this Jul 11, 2022
@shilman shilman merged commit 68eb4fe into storybookjs:next Jul 11, 2022
@Kingdutch Kingdutch deleted the bugfix/support-as-default-csf-export branch July 12, 2022 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants