-
-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Angular: Refactor angular server #14358
Conversation
5c41213
to
c17feae
Compare
5fa4189
to
c22fc7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took a brief look at the code
It looks pretty clean and understandable 🙂
Especially the "webpack config" merge part really needed a refactoring...
@@ -0,0 +1,189 @@ | |||
/** | |||
* This file is to be watched ! | |||
* The code must be compatible from @angular-devkit version 6.1.0 to the latest supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why v6, though?
https://angular.io/guide/releases#support-policy-and-schedule
Angular versions v4, v5, v6, v7, and v8 are no longer under support.
Angular does not support those versions, why should we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
year but storybook still support these old versions no ?
I referred here :
storybook/app/angular/package.json
Line 90 in ea7d051
"@angular-devkit/core": "^0.6.1 || >=7.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shilman @ndelangen What do you think? In the past, I tried to be compatible with the last 4 versions of Angular (8 to 11 right now)
In the link above you can find a table with LTS date ranges. Should we just stick to what Angular does support and write it somewhere down in the docs so we don't need to ask this ourselves over and over again? 🙂
All major releases are typically supported for 18 months.
6 months of active support, during which regularly-scheduled updates and patches are released.
12 months of long-term support (LTS), during which only critical fixes and security patches are released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do it with the next storybook major 7
but not before, no ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we keep back compat until our 7.0 release, but I think we can compromise on semver if it helps clean things up a lot.
}; | ||
|
||
export const filterOutStylingRules = (config: Configuration) => { | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What needs to be ignored here? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummm 🤔 I took it from the old code. I'll go check it out if it's still nessesary
logger.info(`=> Using angular project '${fondProject.projectName}' for configuring Storybook`); | ||
} catch (error) { | ||
logger.error(`=> Could not find angular project`); | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have introduced a change in behavior here 🤔
I throw a lot more "error" than before
To avoid the change of behavior it's necessary to ignores the errors if the project
is not found and if it does not contain architect.build
. with warn
but I wonder if this is a good thing 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If certain setups do not make sense at all I think it's ok to introduce a breaking change. In my experience, misconfiguring storybook may not lead to startup errors but runtime errors instead so it's not even breaking, it is "exit what should not work anyway"
thank @kroeder :) |
@ThibaudAV is this ready to merge? |
@shilman not yet I tell you when it feels right |
…hsPlugin with tsConfig
Adds tests in order to rework the code in next commit without changing the angular preset behavior
c22fc7f
to
a91d9f9
Compare
Rework the code of angular-cli_config and angular-cli_utils to add future features Improvement: - Add test for NX - Use angular core to read the workspace instead of doing it by hand - Use angular-cli to read the tsconfig - Redesigned the code to get out the main steps + added error handling - Express more clearly the webpack config from angular-cli - Clarification of the logs - Improvement of the types with those of angular-cli
a91d9f9
to
4f00418
Compare
@shilman it's ok for me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did only a light review bug LGTM 💯
Issue: not yet. but if all goes well i would like to eventually allow storybook to be more "Angular friendly". And to use an angular builder like in NX to simplify the use of storybook in angular
What I did
Rework the code of
angular-cli_config
andangular-cli_utils
to add future featuresImprovement:
Add test for different workspace configuration
These new tests allow to make sure of the compatibility with angular-cli 🤞. And to avoid (like last time) to update the supported angular version without failing
Use angular core to read the workspace instead of doing it by hand
Use angular-cli to read the tsconfig
Redesigned the code to get out the main steps + added error handling
Express more clearly the webpack config from angular-cli
Clarification of the logs
Improvement of the types with those of angular-cli
I find that it was not easy to understand the code before. I hope to improve it. but if there are still unclear points, do not hesitate to ask questions or suggest improvements.
Test campagne
To avoid regressions I added tests before. And check that after refactoring the tests still work without major changes
Step :
npx @angular/cli@6 new angular-v6
)npx sb init
)dist
in node_modules of the new project(Skip 7 and 8 🙈 )
How to test
Yes
Yes
Yes