Moving conference init to react (rebased) #1204

idainatovych · 2016-12-16T13:53:55Z

Rebased and edited version of #1201

paweldomas · 2016-12-29T14:55:25Z

There is a problem where the original URL stored by the ConferenceUrl is stripped from the parameters. When I enter pawel.jitsi.net/someroom?arg1=234#config.blabla=2 I see in the logs:

[modules/URL/ConferenceUrl.js] : Stored original conference URL: https://pawel.jitsi.net/someroom
app.bundle.js:83346 [modules/URL/ConferenceUrl.js] : Conference URL for invites: https://pawel.jitsi.net/someroom

But it should say:

[modules/URL/ConferenceUrl.js] : Stored original conference URL: https://pawel.jitsi.net/someroom?arg1=234#config.blabla=2
app.bundle.js:83346 [modules/URL/ConferenceUrl.js] : Conference URL for invites: https://pawel.jitsi.net/someroom

It's possible that the "jwt" token param could be lost as well, but I haven't tried that. I guess that it gets lost during routing here, but my knowledge about react is limited and you will probably know better.

idainatovych · 2017-01-04T17:06:41Z

@paweldomas Updated this PR with the changes related to stripping URL query parameters.

…hub.com/BeatC/jitsi-meet into BeatC-BeatC-moving-conference-init-to-react-1

lyubomir · 2017-01-09T14:10:10Z

react/features/app/actions.web.js

+    return () => {
+        init();
+    };
+}


The file react/features/app/actions.web.js is practically a copy of 158 lines from react/features/app/actions.native.js with the addition of a simple function. As per the coding style, this is unacceptable.

The coding style requires sorting by alphabetical order.

lyubomir · 2017-01-09T14:14:38Z

react/features/app/functions.web.js

+            url
+                = match[1] /* URL protocol */
+                + '://enso.hipchat.me/'
+                + url.substring(regex.lastIndex);


The file react/features/app/functions.web.js is practically a copy of a 117 lines from react/features/app/functions.native.js with the additions of functions. As per the coding style, this is unacceptable.

The copied parts have been modified with incorrect indentations.

lyubomir · 2017-01-09T14:21:41Z

react/features/base/lib-jitsi-meet/actions.web.js

+
+        // XXX Temporary solution. Until conference.js hasn't been moved
+        // to the react app we shouldn't use JitsiMeetJS from react app.
+        return Promise.resolve();


The file react/features/base/lib-jitsi-meet/actions.web.js is practically a copy of ~55 lines from react/features/base/lib-jitsi-meet/actions.native.js with a modification of ~10 lines. As per the coding style, this is unacceptable.

lyubomir · 2017-01-09T14:26:13Z

react/features/welcome/components/WelcomePage.web.js

+        this._updateRoomname();
+    }
+
+    /**


I personally removed the method _onUpdateRoomname and its uses. You've brought back the method but not its uses.

lyubomir · 2017-01-09T14:28:08Z

react/features/conference/route.js


 /**
 * Register route for Conference (page).
 */
 RouteRegistry.register({
    component: Conference,
-    path: '/:room'
+    path: '/:room',
+    onEnter: () => {


The coding style requires sorting by alphabetical order.

lyubomir · 2017-01-09T14:28:19Z

react/features/welcome/route.js

 /**
 * Register route for WelcomePage.
 */
 RouteRegistry.register({
    component: WelcomePage,
-    path: '/'
+    path: '/',
+    onEnter


The coding style requires sorting by alphabetical order.

lyubomir · 2017-01-10T20:46:34Z

react/features/conference/actions.js

+
+    // Initialize the conference URL handler
+    APP.ConferenceUrl = new ConferenceUrl(window.location);
+}


The file react/features/conference/actions.js is not used!

It was copied into react/features/conference/functions.js and modified further.

lyubomir · 2017-01-10T20:57:43Z

react/features/conference/functions.js

+        APP.settings.setAvatarUrl((avatarUrl || '').trim());
+        APP.settings.setDisplayName((name || '').trim(), true);
+    }
+}


I believe you're not getting the idea of the file functions.js of a feature. It's supposed to (1) export functions out of the feature and/or (2) declare feature-internal/private functions that are used in multiple other files. If you're using the function of a feature only in one file of the same feature, then the function should be in the filewhich uses it and it should be private to that file (most likely).

lyubomir · 2017-01-10T21:14:44Z

react/features/welcome/route.js

+ * @returns {void}
+ */
+function onEnter(nextState, replace) {
+    if (!APP.settings.isWelcomePageEnabled()) {


Both react/features/welcome/route.js and react/features/conference/route.js are supposed to be platform-agnostic (as the simple .js file extension suggests). You put APP and/or imported from modules/ in them which cannot possibly run on mobile.

lyubomir · 2017-01-10T22:38:56Z

react/features/app/actions.js

+ *
+ * @returns {Function}
+ */
+export function appInit() {


I think this "action" is pointless and I believe it should be removed:

It is dispatched by App.web.js on componentWillMount. However, AbstractApp dispatches appWillMount on componentWillMount. Whatever App.web.js does with appInit can already be done on appWillMount.

It's not really a Redux action that anyone else can intercept or reduce so calling it a Redux action is superfluous.

I'm fully agree with you. It's a temporary solution because app initialization is not fully integrated in new app. And in one of further PRs based on this one I refactored this action to just function.

@lyubomir And also as far as I understand from the codebase we shouldn't initialize the conference until we move to the Conference (in old app we did initialization on the WelcomePage but it was senseless because we did it again when user went to the conference because page was reloaded and all app initialization was run again). In new app we're trying to get rid of page reloading because it improves the UX.

idainatovych mentioned this pull request Dec 19, 2016

Extract conference connector #1207

Closed

idainatovych force-pushed the BeatC-moving-conference-init-to-react-1 branch from fb5b087 to 6f2861d Compare December 26, 2016 12:52

idainatovych mentioned this pull request Dec 27, 2016

Mobile landing #1213

Closed

idainatovych added 7 commits January 4, 2017 17:45

moved app initialization to react app

c570b80

Move redundant code from app.js

3190bfa

Fix lint errs

e716c17

Introduced new actions and functions for app initialization

f53fb3d

Clean up Conference component

a99bbe6

editions after rebase

58b5e17

remove asterisk symbol from jsdoc

ed1aa70

idainatovych force-pushed the BeatC-moving-conference-init-to-react-1 branch from 6f2861d to ed1aa70 Compare January 4, 2017 15:48

Merge branch 'master' into BeatC-moving-conference-init-to-react-1

c275746

lyubomir mentioned this pull request Jan 4, 2017

Moving conference init to react #1201

Closed

Fix url params stripping

ad9bdf4

lyubomir self-assigned this Jan 4, 2017

Merge branch 'BeatC-moving-conference-init-to-react-1' of https://git…

b679942

…hub.com/BeatC/jitsi-meet into BeatC-BeatC-moving-conference-init-to-react-1

lyubomir reviewed Jan 9, 2017

View reviewed changes

lyubomir added 2 commits January 10, 2017 13:06

Remove duplication, simplify, comply with coding style

2f01746

Merge branch 'master' into BeatC-BeatC-moving-conference-init-to-react-1

4d5f826

lyubomir reviewed Jan 10, 2017

View reviewed changes

lyubomir force-pushed the BeatC-moving-conference-init-to-react-1 branch from 6f2ed66 to f1ed580 Compare January 11, 2017 19:52

lyubomir added 2 commits January 12, 2017 10:37

Remove duplication, simplify, comply with coding style

0912dbf

Merge branch 'master' into BeatC-BeatC-moving-conference-init-to-react-1

0936d54

lyubomir force-pushed the BeatC-moving-conference-init-to-react-1 branch from f1ed580 to 0936d54 Compare January 12, 2017 16:38

eslint 3.13.1, react 15.4.1, react-redux 5.0.2

1f995cf

lyubomir merged commit 1f995cf into jitsi:master Jan 12, 2017

GTVolk mentioned this pull request Oct 1, 2023

[Snyk] Security upgrade css-loader from 3.6.0 to 5.0.0 DevVault/jitsi-meet#26

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving conference init to react (rebased) #1204

Moving conference init to react (rebased) #1204

idainatovych commented Dec 16, 2016

paweldomas commented Dec 29, 2016

idainatovych commented Jan 4, 2017

lyubomir Jan 9, 2017 •

edited

Loading

lyubomir Jan 10, 2017

lyubomir Jan 9, 2017

lyubomir Jan 9, 2017

lyubomir Jan 9, 2017

lyubomir Jan 9, 2017

lyubomir Jan 9, 2017

lyubomir Jan 10, 2017

lyubomir Jan 10, 2017

lyubomir Jan 10, 2017

lyubomir Jan 10, 2017

idainatovych Jan 11, 2017

idainatovych Jan 11, 2017

Moving conference init to react (rebased) #1204

Moving conference init to react (rebased) #1204

Conversation

idainatovych commented Dec 16, 2016

paweldomas commented Dec 29, 2016

idainatovych commented Jan 4, 2017

lyubomir Jan 9, 2017 • edited Loading

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

lyubomir Jan 9, 2017 •

edited

Loading