-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Ember: Remove usage of global Ember variable #27186
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
base: next
Are you sure you want to change the base?
Conversation
The Ember global has been deprecated for quite a while now and the ember storybook framework addon relied on a hack on ember-cli-storybook to set up the global again.
This change removes the need for the Ember global by changing the way handlebars templates are precompiled and relying on a story component from ember-cli-storybook (or the user’s application) to render the actual story.
The storybook/story component can be as simple as this:
```
import Component from '@ember/component';
export default Component.extend({});
```
|
We just found out that, since the new // components/storybook/story.js
import Component from '@ember/component';
import { service } from '@ember/service';
export default Component.extend({
store: service('store'),
didInsertElement() {
this.setProperties(this.setupStory() ?? {});
},
setupStory() {
return {};
}
});// story.stories.js
export const Default = Template.bind({});
Default.args = {
setupStory() {
return {
user: this.store.findRecord('user', 1)
};
}
}; |
|
@benedikt Sorry for the massive delay on this. I've resolved the conflicts with the current branch, but am unable to push them to your remote. |
|
@shilman Thanks a lot for this. Much appreciated! Just sent you an invite with Write access to our fork. |
|
View your CI Pipeline Execution ↗ for commit 44d5954
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 12 | 12 | 0 |
| Self size | 2.42 MB | 2.12 MB | 🎉 -303 KB 🎉 |
| Dependency size | 8.93 MB | 8.93 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 31.75 MB | 39.97 MB | 🚨 +8.22 MB 🚨 |
| Dependency size | 17.43 MB | 17.43 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 202 | 213 | 🚨 +11 🚨 |
| Self size | 28 KB | 30 KB | 🚨 +2 KB 🚨 |
| Dependency size | 27.95 MB | 31.21 MB | 🚨 +3.26 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
sb
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 52 | 52 | 0 |
| Self size | 1 KB | 1 KB | 0 B |
| Dependency size | 49.18 MB | 57.40 MB | 🚨 +8.22 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 217 | 217 | 0 |
| Self size | 582 KB | 580 KB | 🎉 -2 KB 🎉 |
| Dependency size | 94.81 MB | 106.58 MB | 🚨 +11.77 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 186 | 186 | 0 |
| Self size | 31 KB | 31 KB | 0 B |
| Dependency size | 78.91 MB | 87.13 MB | 🚨 +8.22 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 1 | 1 | 0 |
| Self size | 12.50 MB | 16.06 MB | 🚨 +3.55 MB 🚨 |
| Dependency size | 98 KB | 98 KB | 0 B |
| Bundle Size Analyzer | Link | Link |
|
@gossi I think you're working on a PR, that negates the need for this one, correct? |
|
At the time this PR was created it was legit. Today it is still legit PR but no longer for the Ember build systems and their compatibility with storybook:
Storybook 10 is ESM only, which breaks compat with older Ember build systems. I'm working on the vite system. So this PR is still worthy but time and context has changed since it has been started, please re-evaluate. I notice this PR allows to use Ember with Storybook and mock a lot of conecpts necessary to make effective use of storybook at all. So even if I might be able to land support Storybook for ember vite, this is still worthy for those companies who bet on Storybook + Ember year(s) ago and will be stuck with storybook v9 + their Ember build system until finishing the migration. |
This pull request is an effort to remove the usage of the
Emberglobal variable from@storybook/ember.Motivation
The global
Embervariable has been deprecated since 2021, but #103 added a workaround to keep@storybook/emberworking with it.However, with Ember’s new build system (Embroider) on the horizon, the workaround is about to stop working. In fact, it’s already causing problems when Ember apps are built using Embroider’s
staticEmberSource: trueconfiguration.Approach
The current implementation relies on the
Emberglobal in two locations. It usesEmber.Componentdirectly (as a wrapping component for the rendered story), andEmber.HTMLBars.templateas a factory for precompiled Handlebars templates.Our approach to remove the usage of the
Emberglobal is based on two ideas:Ember.Componentdirectly in@storybook/ember, we create a<Storybook::Story />component in@storybook/ember-cli-storybookand load it in@storybook/emberusing Ember’s dependency injection mechanism.Ember.HTMLBars.templateto wrap the precompiled templates, we’re usingcreateTemplateFactoryfrom@ember/template-factory.Changes to
@storybook/emberRemoving
babel-plugin-ember-modules-api-polyfillThis babel plugin was responsible for replacing any imports from Ember packages with calls to the
Emberglobal.By removing the plugin, this transformation doesn’t happen anymore.
Replacing
babel-plugin-htmlbars-inline-precompilewithbabel-plugin-ember-template-compilationThese two babel plugins essentially do the same thing. They transform any
precompileTemplate, orhbstagged template strings into precompiled handlebars templates. However,babel-plugin-htmlbars-inline-precompileis considered deprecated, so it felt like a good opportunity to swap it with the supported version.We also tweak the configuration a little bit to transform files within certain packages in
node_modules. This way we can also precompile templates within Ember itself. While there aren’t a lot, they need to be precompiled at build time, otherwise things will break at runtime.Adding package aliases for
@ember/*,@glimmer/*, and other dependencies.With
babel-plugin-ember-modules-api-polyfillremoved, any imports from@ember/*packages will fail because they don’t exists as published packages. Instead they are shipped withinembers-sourceand out of reach. Adding these aliases makes the imports work.Adding
babel-plugin-debug-macrosThings already work fine in the development mode (
storybook dev) with the above changes. However, creating static builds (storybook build) fails, because it tries to import debug packages like@glimmer/debugthat don’t ship code for production environments.To fix this, we need to add
babel-plugin-debug-macros. This way we can set theDEBUGflag of@glimmer/envtofalsein production builds. Tree-shaking will take care of the rest and remove the dependency.Changes to
@storybook/ember-cli-storybookAdding the
Storybook::StorycomponentAs
@storybook/embernow tries to load the wrapping component from the applications dependency injection mechanism, we actually need to provide a component. In theory, this can be done in the user’s application, but why not be nice and ship it withember-cli-storybookdirectly? The component is a classic Ember component. Unfortunately, we couldn’t figure out a way to render a Glimmer component into an HTML element. However, this is also the way Ember’s own rendering tests are currently doing this, so we figured it’s the way to go.Removing the
Emberglobal workaroundAs a last step, we remove the
window.Ember = require("ember").default;workaround from the build process for.storybook/preview-head.html. We also removeEmber.testing=true;, because that doesn’t work anymore without the global.This leaves
<script>runningTests = true</script>in the generatedpreview-head.htmlfile. This global is still required to prevent the Ember application from attaching itself to the root component and rendering itself.We tried to replicate the same behavior using
deferReadiness(), but couldn’t make it work.Known Issues
With the changes above, Storybook builds and runs as a server, provided the stories are simple and don't rely on app files or app state.
Unfortunately, there are a couple of known issues we discovered so far.
Importing Ember packages from within stories
While importing code from
@ember/*packages works, the code doesn’t necessarily work as expected.One example is importing
getOwnerfrom@ember/applicationfrom within a story. ThegetOwnerfunction will exist, but it will be different from the one used inside the Ember application. As a result, callinggetOwneron an object instance from the application will not return the application’s owner.A similar problem exists with
onfrom@ember/object/evented. The function will exist, but it will not have any effect when attached to an instance from the application. For example, anon('init')in the context of a story would previously be called when the story gets rendered. After these changes, it will silently fail and not run at all.The only possible workaround we found so far is gain relying on the global
requireand loading things likegetOwnerandonvia that way:Importing from the application itself
While simple files are not a problem, Storybook will fail to build when the files include imports from
@ember/*packages (i.e.@ember/string).Short term improvements
There’s an upcoming change im Ember that changes the folder structure of
ember-source/distso that there isn’t a difference between packages and dependencies anymore. It apparently also will link the packages between themselves using relative paths, which could help with some of the issues we encountered when it comes to imports.The change will also eventually remove
defineandrequirefromglobal, so the currentloadEmberAppfunction will probably need to change in the near future.Long term solution
We currently think that most of the problems come from the fact that there are two build processes in place:
global.requireand booted within the preview iframe.Previously, this was not an issue, because everything relied on the
Emberglobal from the app’s build process and therefore sharing the same code and the same instances.We believe that the only viable long term solution is getting rid of the initial build process of the app by incorporating it with the story build process itself. This would potentially also remove the need for the
@storybook/ember-cli-storybookpackage, because all its responsibilities would be handled by@storybook/emberitself.It sounds like this should be possible with Embroider, but we haven’t explored the details, yet. It might also make sense to start this effort as a separate Storybook renderer and builder. There’s likely not going to be a lot of overlap with the current implementation of
@storybook/ember.Acknowledgements
Thanks to @NullVoxPopuli for getting us unstuck with the debug macro and package aliases. Thanks to @gossi for his work on Hokulea’s storybook explorer, which inspired some of the ideas in this PR.