Hirad/Made cfd an independent monorepo #5036

hirad-deriv · 2022-03-14T13:25:10Z

Changes:

Please include a summary of the change and which issue is fixed below:

...

When you need to add unit test

If this change disrupt current flow
If this change is adding new flow

When you need to add integration test

If components from external libraries are being used to define the flow, e.g. @deriv/components
If it relies on a very specific set of props with no default behavior for the current component.

Test coverage checklist (for reviewer)

Ensure utility / function has a test case
Ensure all the tests are passing

Type of change

vercel · 2022-03-14T13:25:15Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/deriv/deriv-app/HUpuAmoHZPxashLs7fosSXaKVYi1
✅ Preview: https://deriv-app-git-fork-hirad-rewok-cfd-monorepo-package.binary.sx

github-actions · 2022-03-14T13:36:18Z

A production App ID was automatically generated for this PR. (log)

PR: Hirad/Made cfd an independent monorepo #5036
URLs:
- w/ App ID + Server: https://deriv-app-git-fork-hirad-rewok-cfd-monorepo-package.binary.sx?qa_server=frontend.binaryws.com&app_id=31359
- Original: https://deriv-app-git-fork-hirad-rewok-cfd-monorepo-package.binary.sx
App ID: 31359

Click here to copy & paste above information.

- **PR**: [https://github.com/binary-com/deriv-app/pull/5036](https://github.com/binary-com/deriv-app/pull/5036)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-hirad-rewok-cfd-monorepo-package.binary.sx?qa_server=frontend.binaryws.com&app_id=31359
    - **Original**: https://deriv-app-git-fork-hirad-rewok-cfd-monorepo-package.binary.sx
- **App ID**: `31359`

… cfd-monorepo-package

balakrishna-deriv · 2022-03-17T02:07:57Z

Please add CFD to CODEOWNERS file

balakrishna-deriv · 2022-03-17T02:09:06Z

packages/cfd/.travis.yml

@@ -0,0 +1,11 @@
+sudo: false


we don't use travis anymore. It's safe to remove this

balakrishna-deriv · 2022-03-17T02:11:06Z

packages/cfd/build/constants.js

+
+const ALIASES = {
+    _common: path.resolve(__dirname, '../src/_common'),
+    Assets: path.resolve(__dirname, '../src/Assets'),


Assets folder is not there in cfd/src. I think we should remove it.

balakrishna-deriv · 2022-03-17T02:13:00Z

packages/cfd/globals.d.ts

+declare module '@deriv/bot-web-ui';
+declare module '@deriv/cashier';
+declare module '@deriv/components';
+declare module '@deriv/dashboard';


dashboard is changed to appstore. Can you update it here?

balakrishna-deriv · 2022-03-17T02:16:07Z

packages/core/package.json

@@ -95,6 +95,7 @@
        "@deriv/bot-web-ui": "^1.0.0",
        "@deriv/cashier": "^1.0.0",
        "@deriv/components": "^1.0.0",
+        "@deriv/cfd": "^1.0.0",


you should also add cfd scope to build:travis package command

balakrishna-deriv · 2022-03-17T02:17:27Z

packages/trader/src/Modules/CFD/index.js

@@ -1,5 +0,0 @@
-import CFDDashboard from './Containers/cfd-dashboard';


Can we remove the entire CFD folder in Modules?

It is already done but for some reason github shows it differently.

balakrishna-deriv · 2022-03-17T02:18:36Z

packages/cfd/package.json

+    "@deriv/api-types": "1.0.48",
+    "@deriv/components": "^1.0.0",
+    "@deriv/deriv-api": "^1.0.8",
+    "@deriv/deriv-charts": "^0.5.1",


@deriv/deriv-charts is not needed in this package

matin-deriv · 2022-03-17T02:57:33Z

packages/cfd/src/sass/app.scss

+// Components
+@import 'app/_common/components/amount';
+@import 'app/_common/components/allow-equals';
+//@import 'app/_common/components/calendar'; // TODO: [move-to-components] Calendar component should be moved


You may remove the TODOs in this file.

matin-deriv · 2022-03-17T03:10:34Z

packages/core/src/App/Constants/routes-config.js

+        {
+            path: routes.dxtrade,
+            component: props => <CFD {...props} platform='dxtrade' />,
+            // Don't use `Localize` component since native html tag like `option` cannot render them


we can remove this to decrease duplication..
same for line 59.

yashim-deriv · 2022-03-17T04:10:49Z

packages/cfd/build/constants.js

+    // new HtmlWebPackPlugin(htmlOutputConfig()),
+    // new HtmlWebpackTagsPlugin(htmlInjectConfig()),


Are we going to enable these plugins in the future?

I don't think so. I remove them in the my new commit.

yashim-deriv · 2022-03-17T04:15:03Z

packages/cfd/src/Components/Routes/binary-routes.jsx

+                    <div>
+                        <Localize i18n_default_text='Loading...' />
+                    </div>


Are these divs necessary?

Since fallback is required in react suspense, we are using plain text to avoid further issues.

yashim-deriv · 2022-03-17T04:16:37Z

packages/cfd/src/Components/Routes/binary-routes.jsx

+        >
+            <Switch>
+                {getRoutesConfig({ is_dashboard }).map((route, idx) => (
+                    <RouteWithSubRoutes key={idx} {...route} {...props} />


Suggested change

<RouteWithSubRoutes key={idx} {...route} {...props} />

<RouteWithSubRoutes key={idx} {...route} {...props} />

The key should not use idx generated by map() can we use something unique from the return of getRoutesConfig

maryiafrantsava-deriv · 2022-03-17T09:40:01Z

packages/cfd/src/index.tsx

@@ -0,0 +1,15 @@
+// import 'babel-polyfill';


Why do we need commented import?

I remove it in my last commit.

… cfd-monorepo-package

…/deriv-app into cfd-monorepo-package

maryiafrantsava-deriv · 2022-03-17T09:55:27Z

packages/cfd/src/Components/Routes/__tests__/route-with-sub-routes.spec.js

+import { expect } from 'chai';
+import { configure, shallow } from 'enzyme';
+import Adapter from 'enzyme-adapter-react-16';
+import { RouteWithSubRoutesRender } from '../route-with-sub-routes.jsx';


i guess we can don’t use extension .jsx

Suggested change

import { RouteWithSubRoutesRender } from '../route-with-sub-routes.jsx';

import { RouteWithSubRoutesRender } from '../route-with-sub-routes';

maryiafrantsava-deriv · 2022-03-17T09:59:07Z

packages/cfd/src/Components/Routes/binary-routes.jsx

+import { PlatformContext } from '@deriv/shared';
+import { Localize } from '@deriv/translations';
+import getRoutesConfig from '../../Constants/routes-config';
+import RouteWithSubRoutes from './route-with-sub-routes.jsx';


Suggested change

import RouteWithSubRoutes from './route-with-sub-routes.jsx';

import RouteWithSubRoutes from './route-with-sub-routes';

maryiafrantsava-deriv · 2022-03-17T10:03:53Z

packages/cfd/src/Containers/cfd-top-up-demo-modal.tsx

@@ -1,5 +1,5 @@
 import React from 'react';
-import SuccessDialog from 'App/Containers/Modals/success-dialog.jsx';
+import SuccessDialog from 'Components/success-dialog.jsx';


Suggested change

import SuccessDialog from 'Components/success-dialog.jsx';

import SuccessDialog from 'Components/success-dialog';

Made the suggested changes.

maryia-deriv-archive · 2022-03-21T08:02:52Z

packages/cfd/build/config.js

+          },
+});
+
+const htmlInjectConfig = () => ({


I suggest removing htmlInjectConfig() because it's not used in packages/cfd/build/constants.js same like I was recommended to do it with trader package in my PR #5063 recently

matin-deriv

Please help to add the newly created Package to extract-string.js file to make sure we pick up on the newly added strings and translate them.

sonarcloud · 2022-03-27T09:50:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
29 Code Smells

No Coverage information
4.8% Duplication

matin-deriv

LGTM, let's wait for more approvals...
Noticed there are conflicts on your PR.

cfd is a new mono repo

d54d87e

hirad-deriv requested review from matin-deriv, balakrishna-deriv, akmal-deriv, nijil-deriv and mehrzad-fs as code owners March 14, 2022 13:25

boring-cyborg bot added the Docs label Mar 14, 2022

vercel bot deployed to Preview March 14, 2022 13:25 View deployment

fix--Added cfd to circle ci config

c4fadfa

vercel bot deployed to Preview March 14, 2022 13:35 View deployment

hirad-deriv and others added 2 commits March 15, 2022 10:41

Merge branch 'master' of https://github.com/binary-com/deriv-app into…

59c84c2

… cfd-monorepo-package

Merge branch 'master' into cfd-monorepo-package

22c1960

vercel bot deployed to Preview March 15, 2022 07:28 View deployment

balakrishna-deriv reviewed Mar 17, 2022

View reviewed changes

matin-deriv reviewed Mar 17, 2022

View reviewed changes

yashim-deriv reviewed Mar 17, 2022

View reviewed changes

maryiafrantsava-deriv reviewed Mar 17, 2022

View reviewed changes

hirad-deriv added 4 commits March 17, 2022 13:13

Merge branch 'master' of https://github.com/binary-com/deriv-app into…

0e74707

… cfd-monorepo-package

fix--Made changes according to the recommendations

a93e2ee

Auto syling is done

a671e87

Merge branch 'cfd-monorepo-package' of https://github.com/hirad-rewok…

d4cc19e

…/deriv-app into cfd-monorepo-package

vercel bot deployed to Preview March 17, 2022 10:00 View deployment

maryiafrantsava-deriv suggested changes Mar 17, 2022

View reviewed changes

fix--Made some changes in the file imports

068a24c

vercel bot deployed to Preview March 17, 2022 13:27 View deployment

maryia-deriv-archive reviewed Mar 21, 2022

View reviewed changes

matin-deriv reviewed Mar 21, 2022

View reviewed changes

added cfd to extract-string

74e1b84

vercel bot deployed to Preview March 27, 2022 09:45 View deployment

matin-deriv approved these changes Mar 28, 2022

View reviewed changes

hirad-deriv closed this May 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hirad/Made cfd an independent monorepo #5036

Hirad/Made cfd an independent monorepo #5036

hirad-deriv commented Mar 14, 2022

vercel bot commented Mar 14, 2022 •

edited

Loading

github-actions bot commented Mar 14, 2022 •

edited

Loading

balakrishna-deriv commented Mar 17, 2022

balakrishna-deriv Mar 17, 2022

hirad-deriv Mar 17, 2022

balakrishna-deriv Mar 17, 2022

balakrishna-deriv Mar 17, 2022

balakrishna-deriv Mar 17, 2022

balakrishna-deriv Mar 17, 2022

hirad-deriv Mar 17, 2022

balakrishna-deriv Mar 17, 2022

matin-deriv Mar 17, 2022

hirad-deriv Mar 17, 2022

matin-deriv Mar 17, 2022

yashim-deriv Mar 17, 2022

hirad-deriv Mar 17, 2022

yashim-deriv Mar 17, 2022

hirad-deriv Mar 17, 2022

yashim-deriv Mar 17, 2022

maryiafrantsava-deriv Mar 17, 2022

hirad-deriv Mar 17, 2022

maryiafrantsava-deriv Mar 17, 2022

maryiafrantsava-deriv Mar 17, 2022

maryiafrantsava-deriv Mar 17, 2022

hirad-deriv Mar 17, 2022

maryia-deriv-archive Mar 21, 2022

matin-deriv left a comment

sonarcloud bot commented Mar 27, 2022

matin-deriv left a comment •

edited

Loading

		@@ -1,5 +0,0 @@
		import CFDDashboard from './Containers/cfd-dashboard';

		// new HtmlWebPackPlugin(htmlOutputConfig()),
		// new HtmlWebpackTagsPlugin(htmlInjectConfig()),

	<RouteWithSubRoutes key={idx} {...route} {...props} />
	<RouteWithSubRoutes key={idx} {...route} {...props} />

	import { RouteWithSubRoutesRender } from '../route-with-sub-routes.jsx';
	import { RouteWithSubRoutesRender } from '../route-with-sub-routes';

	import RouteWithSubRoutes from './route-with-sub-routes.jsx';
	import RouteWithSubRoutes from './route-with-sub-routes';

	import SuccessDialog from 'Components/success-dialog.jsx';
	import SuccessDialog from 'Components/success-dialog';

Hirad/Made cfd an independent monorepo #5036

Hirad/Made cfd an independent monorepo #5036

Conversation

hirad-deriv commented Mar 14, 2022

Changes:

When you need to add unit test

When you need to add integration test

Test coverage checklist (for reviewer)

Type of change

vercel bot commented Mar 14, 2022 • edited Loading

github-actions bot commented Mar 14, 2022 • edited Loading

balakrishna-deriv commented Mar 17, 2022

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

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

matin-deriv left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 27, 2022

matin-deriv left a comment • edited Loading

Choose a reason for hiding this comment

vercel bot commented Mar 14, 2022 •

edited

Loading

github-actions bot commented Mar 14, 2022 •

edited

Loading

matin-deriv left a comment •

edited

Loading