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

[WIP] Angular versions support #2467

Merged
merged 21 commits into from
Dec 20, 2017
Merged

[WIP] Angular versions support #2467

merged 21 commits into from
Dec 20, 2017

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Dec 12, 2017

Issue: Things from #269

What I did

  1. Created util that patches different angular versions
  2. Moved angular deps to peer

Tested with:

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

"@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",
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this?

Copy link
Member Author

@igor-dv igor-dv Dec 12, 2017

Choose a reason for hiding this comment

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

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

@codecov
Copy link

codecov bot commented Dec 12, 2017

Codecov Report

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

Impacted file tree graph

@@               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.

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
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree

Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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 😞

Copy link
Member

Choose a reason for hiding this comment

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

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".

Copy link
Member Author

Choose a reason for hiding this comment

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

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..

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

@kirtangajjar
Copy link

Just a question -

Is angular supported by storybook?

@igor-dv
Copy link
Member Author

igor-dv commented Dec 15, 2017

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

@igor-dv
Copy link
Member Author

igor-dv commented Dec 17, 2017

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

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

  1. 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).
  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.
  3. 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
Copy link
Member

alterx commented Dec 20, 2017

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
Copy link
Member Author

igor-dv commented Dec 20, 2017

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

@igor-dv igor-dv merged commit 6c28b98 into release/3.3 Dec 20, 2017
@igor-dv
Copy link
Member Author

igor-dv commented Dec 20, 2017

Accepted in slack and skype =)

@igor-dv igor-dv deleted the angular-versions-support branch December 20, 2017 20:31
@shilman shilman mentioned this pull request Dec 23, 2017
@@ -34,6 +33,7 @@
},
"peerDependencies": {
"@storybook/addons": "^3.3.0-alpha.4",
"@angular/core": "=>4.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

This should be >= not =>

igor-dv added a commit that referenced this pull request Dec 24, 2017
@igor-dv igor-dv mentioned this pull request Dec 24, 2017
@robertbossaert
Copy link

robertbossaert commented Jan 3, 2018

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
Copy link
Member Author

igor-dv commented Jan 3, 2018

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

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 3, 2018

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

@igor-dv
Copy link
Member Author

igor-dv commented Jan 3, 2018

@Hypnosphi 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants