Bar next version #707
Bar next version #707
Conversation
ptbrowne
commented
Oct 19, 2020
•
ptbrowne
commented
- Try to take React and ReactDOM from the app (WIP)
- Use transpiled cozy-ui
There was a problem hiding this comment.
Looks promising but severals remarks:
-
On retire ici la possibilité de continuer à faire évoluer la version de la bar avec React embarqué. J'ai bien compris ? Il me semblait qu'on voulait toujours livrer les 2 versions avec / sans react pour continuer à la faire évoluer partout.
-
Vu qu'on ne "transpile" plus cozy-ui, on doit importer tout le css transpilé depuis UI ? Alors qu'avant, on utilisait que le CSS dont on avait besoin en créant un fichier CSS à la volée. Ça veut dire qu'on part du principe que l'app a bien importé le CSS transpilé de UI et que la bar va "l'utiliser" pour s'afficher correctement ?
-
Le fait d'importer le Sprite complet de UI me fait un peu tiquer, il est costaud quand même (et il me semble qu'on ne puisse pas le mutualiser avec celui de l'app)
package.json
Outdated
| @@ -31,13 +31,15 @@ | |||
| "devDependencies": { | |||
| "@babel/core": "7.6.2", | |||
| "@babel/polyfill": "^7.10.4", | |||
| "@material-ui/core": "4.11.0", | |||
| @@ -8,6 +8,7 @@ import { Route, hashHistory } from 'react-router' | |||
| import { MobileRouter } from 'cozy-authentication' | |||
| import 'cozy-ui/transpiled/react/stylesheet.css' | |||
| import appIcon from './icon.png' | |||
| import { Sprite as IconSprite } from 'cozy-ui/transpiled/react/Icon' | |||
There was a problem hiding this comment.
Not sure about this one. We are now importing the full UI's sprite for 5 icons.
What was the bundle size before and after this change?
There was a problem hiding this comment.
cozy/cozy-ui#1626 As soon as this is merged, we will be able to import only specific icons, and the bar will not rely on the IconSprite.
| const { connect, Provider } = require('react-redux') | ||
| const I18n = require('cozy-ui/react/I18n').default | ||
| const Bar = require('components/Bar').default | ||
| const CozyProvider = require('cozy-client').CozyProvider |
There was a problem hiding this comment.
By removing those lines, we'll not be able to build the old style bar (by old style bar I mean... the bar that injects react by itself)
There was a problem hiding this comment.
Because now, if you load the script bar before your app script, you'll have an error since React will not be exposed yet, no ?
cf https://github.com/cozy/cozy-passwords/blob/master/src/targets/browser/index.ejs / https://github.com/cozy/cozy-sandbox/blob/master/src/targets/browser/index.ejs /
There was a problem hiding this comment.
For the full bundle, it will use the React from the bundle.
For the bundle without React, the cozy-bar must be included after the app.
I think it will simplify our mental model, either you use the full bundle and the bar uses its own React, or it does not include it and it uses the React from the app.
There was a problem hiding this comment.
For the bundle without React, the cozy-bar must be included after the app.
This is my point. It's a BC for a lot of Cozy app's. I don't know if we want to upgrade all of our old apps for that (mesinfos and other apps like that)
There was a problem hiding this comment.
Older apps will not use the bundle without React, this is opt-in.
There was a problem hiding this comment.
OK, I had a misunderstanding on my side between bundled and bundle without react, sorry for that.
I understand the fact we "external" React & ReactDom, I understand that if we inject React in window.React, this window.React will be used but:
-
Is it because any external module will be replaced by window.MyModule by webpack automatically? So we we write
import React from 'react', it'll be transformed to import React from window.react ? -
It seems that we don't use globalReact anymore, so how the bundled bar require React & ReactDom (or ATM called 'my-react')?
There was a problem hiding this comment.
Is it because any external module will be replaced by window.MyModule by webpack automatically? So we we write import React from 'react' , it'll be transformed to import React from window.react ?
The end result is yes, but I think to achieve this, webpack simply creates a "fake" React module that exports window.React. But it has the same end effect as what you wrote.
It seems that we don't use globalReact anymore, so how the bundled bar require React & ReactDom (or ATM called 'my-react')?
It requires it through a normal import. It is not there for the moment in the PR, this might be why it is not clear, but there will be two builds, one with webpackConfig.externals configured to externalize React and ReactDOM and the other without this externals config.
It can make the build fail with no reason
- We use minilog for logs so that we can activate/deactivate logs at runtime - Minilog does not support the group/groupEnd so we cannot use title formatter
TODO It should be possible to wait for the bar script to load completely by using the load event, but it did not work at first, we need to investigate.
TODO Find out the cause of the crash and solve it
Those aliases were used so that the bar used the React from the app if it was provided. Now, in the next version of the bar, either the app chooses to use the bar+react, or it uses only the bar and the bar expects to find React and ReactDOM in the environment.
ptbrowne
force-pushed
the
improvements2
branch
from
de5e48c to
eaa1a2d
Compare
|
I will not be able to continue working on this. |
