[WIP] Angular versions support #2467

igor-dv · 2017-12-12T12:51:27Z

Issue: Things from #269

What I did

Created util that patches different angular versions
Moved angular deps to peer

Tested with:

Version	Status
^5.0.0-beta.7	👌
4.3.5	👌
2.4.10	deprecated

ndelangen · 2017-12-12T13:22:12Z

examples/angular-cli/package.json

-    "@angular/platform-browser": "^5.0.0-beta.7",
-    "@angular/platform-browser-dynamic": "^5.0.0-beta.7",
-    "@angular/router": "^5.0.0-beta.7",
+    "@angular/animations": "4.3.5",


What's the reason for this?

wanted to check if it works with v4 (#269 (comment))

…ar v2

codecov · 2017-12-12T13:55:04Z

Codecov Report

Merging #2467 into release/3.3 will decrease coverage by 0.05%.
The diff coverage is 0%.

@@               Coverage Diff               @@
##           release/3.3    #2467      +/-   ##
===============================================
- Coverage         19.5%   19.45%   -0.06%     
===============================================
  Files              386      387       +1     
  Lines             8709     8734      +25     
  Branches           931      946      +15     
===============================================
  Hits              1699     1699              
- Misses            6301     6303       +2     
- Partials           709      732      +23

Impacted Files	Coverage Δ
app/angular/src/server/config/webpack.config.js	`0% <ø> (ø)`	⬆️
...p/angular/src/server/config/webpack.config.prod.js	`0% <ø> (ø)`	⬆️
addons/knobs/src/angular/helpers.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/angular/utils.js	`0% <0%> (ø)`
app/vue/src/server/config/babel.js	`0% <0%> (-100%)`	⬇️
addons/a11y/src/components/Report/Elements.js	`0% <0%> (ø)`	⬆️
addons/info/src/components/PropTable.js	`29.78% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Boolean.js	`11.62% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/components/layout/index.js	`29.28% <0%> (ø)`	⬆️
... and 53 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 cb34518...cc32f92. Read the comment docs.

igor-dv · 2017-12-13T11:32:25Z

app/angular/src/client/preview/angular/helpers.ts

 import { platformBrowserDynamic } from "@angular/platform-browser-dynamic";
 import { BrowserModule } from "@angular/platform-browser";
 import { AppComponent } from "./components/app.component";
 import { ErrorComponent } from "./components/error.component";
 import { NoPreviewComponent } from "./components/no-preview.component";
 import { STORY } from "./app.token";
+import { getAnnotations, getParameters, getPropMetadata } from './utils';
+
 let platform = null;
 let promises = [];

 // Taken from https://davidwalsh.name/javascript-debounce-function


We already have lodash.debounce in the project. Why not using it.

If we have it available than let's use it. I was under the impression that we didn't :)

igor-dv · 2017-12-13T11:33:48Z

app/angular/src/client/preview/angular/utils.ts

@@ -0,0 +1,28 @@
+function getMeta(component, [name1, name2]: any, defaultValue) {


Where can I put this util ? It's duplicated in two projects: addons/knobs and app/angular

Hmm, @ndelangen, where would be the best place to put this util? 🤔

I was under the impression that there was an ongoing effort to remove duplicate parts of the code to a core package. Dunno if that's still a thing for 3.3 tho. We might just leave this as is for the time being and wait for that core module to be ready (?)

Yeah, I agree

It was kinda decided to leave the refactor until after the initial release of 3.3, and release it as a patch. @shilman started the refactor branch, @Hypnosphi wanted to hold it off, if I remember correctly.

Where can I put this util ? It's duplicated in two projects: addons/knobs and app/angular

Please give me a bit more context so I can answer your question

igor-dv · 2017-12-13T11:40:40Z

app/angular/src/server/config/webpack.config.js

@@ -55,6 +57,16 @@ export default function() {
      new CaseSensitivePathsPlugin(),
      new WatchMissingNodeModulesPlugin(nodeModulesPaths),
      new webpack.ProgressPlugin(),
+      // temp plugin to make webpack bundle only one v5 version.


when changing a version of the example/angular-cli to different than the one in the app/angular or addons/knbos - the v5 is bundled twice and causes to StaticInjectorError something-something. I wonder will it be the same if storybook is used in a separate project (I think yes), should we put this plugin to the code base or document that people will need to add it explicitly

Please correct me if I'm wrong (probably yes) but shouldn't the angular version inside the @storybook/angulars package.json be used as a dep of storybook and the version in the consumer project be used for that (when using storybook in a separate project)?

Anyway, we'll need to test it.

Let's say there is a project with this package.json

{ "dependencies": [ "@angular/core": "^4.x.x" ], "devDependencies": [ "@storybook/angular": "^3.3" "@storybook/addon-knobs": "^3.3" ] }

I assume that the node modules structure will be like this:

node_modules |- angular |-- core (4.x.x) |- storybook |-- angular |--- node_modules |---- angular |----- core (5.x.x) |-- addon-knobs |--- node_modules |---- angular |----- core (5.x.x)

So what webpack will do...

uhm, shouldn't @angular/core be a peerDependency to prevent exactly that?

@ndelangen, yeah, that was my first implementation. But after, we discussed it in this long conversation. In short, having a peer on angular will force us to be super backward compatible with the old versions, and we won't be able to adopt new angular things. For example, there is a thingy called InjectionToken that used in app/angular. It was introduced in v4 instead of the old OpaqueToken from v2 (that became deprecated in v4). In v5 the OpaqueToken was completely removed. In this case, with a peer on angular, we can't support the v2 (or we need to add some webpack replacement hooks). The alternative is to depend on the latest angular version, but in this case, we end up with 2 angular versions in the bundle (or 3 because of the described above). I hope I was clear 😞

Well to be honest, I think the situation of "it's a peerDependency and you have to make sure it's a version that's compatible with storybook/angular" is preferable to "it's a regularDependency and you better be using the exact same version or you're f%*&ed".

So if returning it back to peer we will have to deprecate angular 2. I am ok with this if you ask me, but I'm not familiar with the adoption rate of angular..

Is there any way to do some kind of feature detection and pick between OpaqueToken and InjectionToken based on what's available in actual angular version?

That what I've tried to do with webpack replacement, but turned out of this way. I think it won't scale if will need to patch every breaking change...

…n` warning with angular 5.

…ns-support # Conflicts: # yarn.lock

kirtangajjar · 2017-12-15T08:40:16Z

Just a question -

Is angular supported by storybook?

igor-dv · 2017-12-15T12:02:05Z

It will be from the upcoming release 3.3. But you can still try a beta.

igor-dv · 2017-12-17T09:12:32Z

@alterx, @Hypnosphi, @ndelangen, @shilman, @danielduan

I think we need to choose just one of the following ways to continue:

Make angular as a peer dependency, but limit it to the versions >=4 - in this case, we won't support v2, and if we will upgrade to the next versions of angular we will have to deprecate previous versions. (I think with react will be the same, for example, fragments from 16.2).
Make angular as a regular dependency of the latest version in core and all the other relevant places in the monorepo (knobs) - in this case we will be able to support older versions of angular. Two versions of angular will be bundled and we will need to have some webpack replacement trick for this.
Have several apps or releases with specific versions of angular (or other libs if needed). for example: storybook/react-16.2, storybook/angular-2.

I think we need to choose between 1 and 2. We can always change it in the future.

alterx · 2017-12-20T01:00:19Z

My vote is for option number 1. Let's deprecate Angular 2 and start from there. After that, if there's a feature that we really need from newer versions of Angular we can just state in the docs which versions of storybook support which versions of angular or just deprecate old versions.

I think something similar to this was done with React pre-v15

igor-dv · 2017-12-20T07:04:47Z

I think it's ok now. Can someone test it with a few versions too ?

…n` in webpack.config.prod

Signed-off-by: Carlos Vega <clmvega@gmail.com>

…ybook into angular-versions-support

igor-dv · 2017-12-20T20:31:33Z

Accepted in slack and skype =)

Hypnosphi · 2017-12-23T18:27:14Z

addons/knobs/package.json

@@ -34,6 +33,7 @@
  },
  "peerDependencies": {
    "@storybook/addons": "^3.3.0-alpha.4",
+    "@angular/core": "=>4.0.0",


This should be >= not =>

robertbossaert · 2018-01-03T10:14:36Z

Having angular/core inside of the peerDeps will throw a "requires a peer of angular/core" warning on React / Vue projects that do not have angular inside their package.json?

igor-dv · 2018-01-03T10:18:19Z

Hm, yeah that's the problem now if you use knobs. I will change the knobs back to work with the regular dep.

Hypnosphi · 2018-01-03T13:41:12Z

@igor-dv alternatively, you can just remove the peerDeps (users will get their warnings anyway, from the storybook/<framework> package)

igor-dv · 2018-01-03T13:52:46Z

@Hypnosphi 👍

igor-dv added 4 commits December 12, 2017 01:37

Create metadata extraction util

63be049

WIP utils

00106d3

Use utils on a few more helper functions

3778ed9

Update yarn.lock

e4ccd6d

ndelangen reviewed Dec 12, 2017

View reviewed changes

Use deprecated OpaqueToken instead of InjectionToken to support angul…

6e6d36d

…ar v2

danielduan added this to the v3.3.0 milestone Dec 12, 2017

danielduan added angular in progress labels Dec 12, 2017

igor-dv added 2 commits December 13, 2017 12:14

Revert changes back to the strong angular 5 version.

a55741d

Revert back to v5

fef3b98

igor-dv commented Dec 13, 2017

View reviewed changes

igor-dv and others added 4 commits December 14, 2017 08:14

Fix `Critical dependency: the request of a dependency is an expressio…

154e510

…n` warning with angular 5.

Add node to test (for windows)

baaa14b

Merge remote-tracking branch 'origin/release/3.3' into angular-versio…

fc980e7

…ns-support # Conflicts: # yarn.lock

Merge branch 'release/3.3' into angular-versions-support

eb252c7

Merge branch 'release/3.3' into angular-versions-support

68c119e

Merge branch 'release/3.3' into angular-versions-support

0a8c581

alterx mentioned this pull request Dec 20, 2017

Added type annotation to helpers, added ts declaration files for angu… #2459

Merged

igor-dv and others added 4 commits December 20, 2017 06:55

Merge branch 'release/3.3' into angular-versions-support

9cc09db

Move back to peer deps

dd0d185

Remove setMeta

08b0bda

Add required click handler to prevent errors.

a1470f6

Remove pathinfo

24873e7

Fix `Critical dependency: the request of a dependency is an expressio…

6ed2fd8

…n` in webpack.config.prod

igor-dv mentioned this pull request Dec 20, 2017

Angular - Add component story with dependencies #2514

Closed

alterx added 2 commits December 20, 2017 09:09

Updating fixtures to reflect deprecation of angular 2

1ecd649

Signed-off-by: Carlos Vega <clmvega@gmail.com>

Merge branch 'angular-versions-support' of github.com:storybooks/stor…

cc32f92

…ybook into angular-versions-support

igor-dv merged commit 6c28b98 into release/3.3 Dec 20, 2017

igor-dv removed the in progress label Dec 20, 2017

igor-dv deleted the angular-versions-support branch December 20, 2017 20:31

shilman mentioned this pull request Dec 23, 2017

Angular 2 support #269

Closed

Hypnosphi reviewed Dec 23, 2017

View reviewed changes

igor-dv added a commit that referenced this pull request Dec 24, 2017

Change "=>" to ">=" as for @Hypnosphi comment in #2467

da62a44

igor-dv mentioned this pull request Dec 24, 2017

Misc fixes #2556

Merged

Hypnosphi mentioned this pull request Jan 3, 2018

Angular and Vue storyshots #2564

Merged

dependencies bot mentioned this pull request May 14, 2018

Update 80 dependencies from npm #3584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Angular versions support #2467

[WIP] Angular versions support #2467

igor-dv commented Dec 12, 2017 •

edited

Loading

ndelangen Dec 12, 2017

igor-dv Dec 12, 2017 •

edited

Loading

codecov bot commented Dec 12, 2017 •

edited

Loading

igor-dv Dec 13, 2017

alterx Dec 13, 2017

igor-dv Dec 13, 2017

alterx Dec 13, 2017

alterx Dec 13, 2017

igor-dv Dec 13, 2017

ndelangen Dec 14, 2017

igor-dv Dec 13, 2017

alterx Dec 13, 2017

igor-dv Dec 13, 2017

ndelangen Dec 14, 2017

igor-dv Dec 14, 2017

ndelangen Dec 14, 2017

igor-dv Dec 15, 2017

Hypnosphi Dec 15, 2017

igor-dv Dec 15, 2017

kirtangajjar commented Dec 15, 2017

igor-dv commented Dec 15, 2017

igor-dv commented Dec 17, 2017

alterx commented Dec 20, 2017

igor-dv commented Dec 20, 2017

igor-dv commented Dec 20, 2017

Hypnosphi Dec 23, 2017

robertbossaert commented Jan 3, 2018 •

edited

Loading

igor-dv commented Jan 3, 2018

Hypnosphi commented Jan 3, 2018 •

edited

Loading

igor-dv commented Jan 3, 2018 •

edited

Loading

		@@ -0,0 +1,28 @@
		function getMeta(component, [name1, name2]: any, defaultValue) {

[WIP] Angular versions support #2467

[WIP] Angular versions support #2467

Conversation

igor-dv commented Dec 12, 2017 • edited Loading

What I did

Tested with:

Choose a reason for hiding this comment

igor-dv Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2017 • edited Loading

Codecov Report

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

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

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

kirtangajjar commented Dec 15, 2017

igor-dv commented Dec 15, 2017

igor-dv commented Dec 17, 2017

alterx commented Dec 20, 2017

igor-dv commented Dec 20, 2017

igor-dv commented Dec 20, 2017

Choose a reason for hiding this comment

robertbossaert commented Jan 3, 2018 • edited Loading

igor-dv commented Jan 3, 2018

Hypnosphi commented Jan 3, 2018 • edited Loading

igor-dv commented Jan 3, 2018 • edited Loading

igor-dv commented Dec 12, 2017 •

edited

Loading

igor-dv Dec 12, 2017 •

edited

Loading

codecov bot commented Dec 12, 2017 •

edited

Loading

robertbossaert commented Jan 3, 2018 •

edited

Loading

Hypnosphi commented Jan 3, 2018 •

edited

Loading

igor-dv commented Jan 3, 2018 •

edited

Loading