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

perf: move out non-reused components #4051

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

frenautvh
Copy link
Contributor

Question Answer
Branch? develop
Bug fix? no
New feature? no
Breaking change? no
Tickets Fix #...
License BSD 3-Clause

Description

Some modules in cloud-universe-components are used only once. Move them out to the corresponding app or module directly. This helps to avoid bundling unnecessary components.

(for example d3 has been removed from cloud-universe-components and won't be bundled anymore on manager dedicated app)

⚠️ billing/consumption section has been removed from cloud application as i suppose it's dead code, i would like some confirmation to ensure that i'm not missing something here

Note regarding adding breaking change or not to the commit :
I think it's debatable since cloud-universe-components is a private package, semver is basically ignored because at each build the last version will be used. Making the commit a breaking change would also major bump some apps & modules which i think is not what we want (and splitting in 2 commits seems difficult since files are moved and not created/deleted). Please share your point of view if you disagree thanks

@frenautvh frenautvh added performance Improves performance refactor A code change that neither fixes a bug nor adds a feature labels Nov 18, 2020
@frenautvh frenautvh requested a review from a team as a code owner November 18, 2020 14:04
Copy link
Collaborator

@marie-j marie-j left a comment

Choose a reason for hiding this comment

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

Regarding commit history, it would be better to have a BREAKING CHANGE commit but I agree that it doesn't make sense for other modules than cloud-universe-components

@@ -8,11 +8,13 @@ import '@ovh-ux/ng-ovh-doc-url';
import { Environment } from '@ovh-ux/manager-config';

import logs from './logs';
import cuiDualList from './components/dual-list';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we can replace all those cui components by ovh-ui-kit ones as a next step

angular.module(moduleName, [
cucConsumption,
cucCurrency,
cucFlavor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I have a look this is only used for one function of one service across all manager, maybe we can just replace its usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just deleted cucFlavor module and copied the "addOverQuotasInfos" where it's used (in kubernetes node-pool nodes add)

import cucNavigation from './navigation';
import cucOrderedHash from './orderedHash';
import cucPoll from './poll';
import cucPrice from './price';
import cucRegion from './region';
import cucProducts from './products';
import cucSmoothScrollHere from './smoothScrollHere';
import cucSpaceMeter from './space-meter';
import cucSshKeyMin from './sshKeyMin';
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is the same filter in the dedicated app. We have moved it to dedicated universe components.

https://github.com/ovh/manager/blob/master/packages/manager/apps/dedicated/client/app/account/user/components/filters/sshkeyMin/sshkeyMin.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I moved it to dedicated universe componets. We need to revisit to consolidate this.

@frenautvh frenautvh force-pushed the feat/cloud-universe-components-refactor branch from 7b9307d to ef54f74 Compare November 23, 2020 08:30
radireddy
radireddy previously approved these changes Dec 2, 2020
marie-j
marie-j previously approved these changes Dec 2, 2020
mohammed-zahaf
mohammed-zahaf previously approved these changes Dec 21, 2020
cbourgois
cbourgois previously approved these changes Feb 1, 2021
@cbourgois
Copy link
Contributor

Hey @frenautvh there is some conflicts (maybe related with #4306)

@frenautvh frenautvh force-pushed the feat/cloud-universe-components-refactor branch 2 times, most recently from 985b5ea to 13c136f Compare February 5, 2021 08:35
Copy link
Member

@antleblanc antleblanc left a comment

Choose a reason for hiding this comment

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

Well done!

Only noticed two translations files that can be removed.

@frenautvh frenautvh force-pushed the feat/cloud-universe-components-refactor branch from 13c136f to 6d0f640 Compare February 5, 2021 15:16
@antleblanc antleblanc self-requested a review February 5, 2021 15:57
@antleblanc antleblanc changed the title refactor(cloud-universe-components): move out non-reused components perf: move out non-reused components Feb 9, 2021
@antleblanc antleblanc merged commit 62c12a0 into develop Feb 9, 2021
@antleblanc antleblanc deleted the feat/cloud-universe-components-refactor branch February 9, 2021 08:36
@antleblanc antleblanc mentioned this pull request Feb 9, 2021
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Improves performance refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants