-
-
Notifications
You must be signed in to change notification settings - Fork 832
Implementation of new potential skinning mechanism #3723
Conversation
This breaks the tests
We're switching to Jest anyways, and getting these things to run is basically impossible at the moment.
They rely on a working riot-web build, which this is not.
MatrixClientPeg in particular doesn't work very well with this.
22ccf2d
to
f6791fa
Compare
With a switch to Only One Webpack™ we need a way to help developers generate the component index without a concurrent watch task. The best way to do this is to have developers import their components, but how do they do that when we support skins? The answer in this commit is to change skinning. Skinning now expects to receive your list of overrides instead of the react-sdk+branded components. For Riot this means we send over *only* the Vector components and not Vector+react-sdk. Components can then be annotated with the `replaceComponent` decorator to have them be skinnable. The decorator must take a string with the dot path of the component because we can't reliably calculate it ourselves, sadly. The decorator does a call to `getComponent` which is where the important part of the branded components not including the react-sdk is important: if the branded app includes the react-sdk then the decorator gets executed before the skin has finished loading, leading to all kinds of fun errors. This is also why the skinner lazily loads the react-sdk components to avoid importing them too early, breaking the app. The decorator will end up receiving null for a component because of the getComponent loop mentioned: the require() call is still in progress when the decorator is called, therefore we can't error out. All usages of getComponent() within the app are safe to not need such an error (the return won't be null, and developers shouldn't use getComponent() after this commit anyways). The AuthPage, being a prominent component, has been converted to demonstrate this working. Changes to riot-web are required to have this work. The reskindex script has also been altered to reflect these skinning changes - it no longer should set the react-sdk as a parent. The eventual end goal is to get rid of `getComponent()` entirely as it'll be easily replaced by imports.
f6791fa
to
20a6153
Compare
+1000 |
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.
OK - I realised why I was completely failing to understand this which was because I was expecting the replaceComponent
decorator to be on the replacing component, but in fact the decorator presumably goes on both the replaced and the replacing component. Any reason for not having the riot-web side of the PR? I appreciate it's probably trivial but think it might still have helped me understand.
The PR description is also really useful, but if it could find a home in the codebase somewhere it would help people reading the code in future to understand it without having to dig out the original PR.
In general this seems fine. Biggest problem is probably the one you point out with everything exploding if things don't get imported in the right order. It's a shame we can't detect the failure mode better. I think it would be entirely fine, if not desirable even, for the app to explode on load if the imports are in the wrong order.
Having to have the decorators on the components in order for them to be replaceable might be another thing to go wrong and get forgotten but probably isn't the end of the world.
I should say btw that the reskindex running in the build step is a recent addition: previously we just re-ran reskindex whenever a component was added and committed it to git. We could go back to this as a simple way of getting around having nowhere to run the reskindex watcher.
# plugins: | ||
# - docker#v3.0.1: | ||
# image: "node:10" | ||
# propagate-environment: true |
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.
when can they end to end tests come back?
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.
They are removed by a different PR. Plan is shortly.
"transform-runtime", | ||
"add-module-exports", | ||
"syntax-dynamic-import" | ||
["@babel/plugin-proposal-decorators", { "legacy": true }], |
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'm missing why we need legacy decorators, especially given we're only just writing the code now. Shouldn't we be using the stage 2 version?
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.
We use legacy decorators elsewhere - it turns out we need this regardless. The modern decorators also aren't compatible with this sort of thing just yet (we need more ES6 code).
@@ -25,6 +25,7 @@ module.exports = { | |||
parserOptions: { | |||
ecmaFeatures: { | |||
jsx: true, | |||
legacyDecorators: true, |
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.
Just to note it looks like this has been removed in babel-eslint v11 so adding it now seems nonideal.
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.
we have to add it now otherwise eslint complaints aggressively :(
} | ||
|
||
// Just return nothing instead of erroring - the consumer should be smart enough to | ||
// handle this at this point. |
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.
Because the only consumer now is going to be a wrapper or something?
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.
It could still be a component, but null is just as clear as a component missing in terms of blowing up.
This was pointed at the wrong branch ._. |
@dbkr This is only needed on the component to be replaced. The decorator is executed at runtime, and only decorates classes (which may be a problem for some of our codebase, but that bridge hasn't been crossed yet). The lifecycle is roughly:
The decorator here is supposed to be used for adding properties/functions to things. One example is adding a decorator for putting an EventEmitter on something (if you had a limitation that prevented you from just extending it normally, like a third party library). Here we replace the entire class instead, which is technically not what's going on: we're actually overriding the component's members with the skinned component's members. This works similar to a replacement because we ultimately override the constructor, lifecycle handlers, and The end goal is to remove the react-sdk's component index because it doesn't really serve a purpose once we decorate the entire project. The current issue with it is that it needs generating, so though we could remove the watcher and run it manually it's almost certainly going to be forgotten about by developers. If we use imports instead it just magically works and we don't need to worry about "registering" the component in some index somewhere. |
Ah right - understood (presumably if you then wanted to override the overriding classes from a third project though, the ones in riot-web would need to be decorated). I would maybe rename
Yep, I get it - just saying that an alternative would be to go back to running reskindex only when we add components and versioning it, then we only have to run it when adding a component. Yeah, people will still forget at that point but would allow us to fix source maps without having to rewrite the skinning stuff, although it looks like this is mostly done now. Main thing though - could all the excellent explanatory notes and doc in this PR go in tree somewhere, so we don't have to dig this PR out to understand how it works? :) |
Renaming it to something helpful can easily be done. Docs are being written. I have a fear of forgetting how this works myself ._. Sourcemaps aren't necessarily blocked on this, though it is a massive quality of life improvement (in that we don't have to remember to do things). |
We're making an assumption here that the decorator is actually all over the app when it's not.
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.
lgtm then I guess, although still a bit worried about the failure mode on old getComponent calls when the component doesn't exist (ie. react will just start whining about nulls deep within its guts) but I guess this is an incentive to remove all the getComponent()s.
Yup, per the description the plan is to open a subsequent PR that does the conversion for everything else |
#3723 removed the prepare script which was how the SDK got built before being published. Add it back as a more modern prepublish script.
#3723 removed the prepare script which was how the SDK got built before being published. Add it back as a more modern prepublish script.
Reviewer: Please look at this for sanity. If it looks sane, it'll get merged and replicated to the rest of the project. It's an open question if we should apply this to all components or just the ones riot-web overrides.
Based upon #3722 - true diff: pull/3722/head...pull/3723/head
Requires element-hq/element-web#11660
Note: This is set to merge against
travis/sourcemaps
because it breaks all the downstream builds.Also sorry for the long description, this is basically a science experiment.
With a switch to Only One Webpack™ we need a way to help developers generate the component index without a concurrent watch task. The best way to do this is to have developers import their components, but how do they do that when we support skins? The answer in this commit is to change skinning.
Skinning now expects to receive your list of overrides instead of the react-sdk+branded components. For Riot this means we send over only the Vector components and not Vector+react-sdk.
Components can then be annotated with the
replaceComponent
decorator to have them be skinnable. The decorator must take a string with the dot path of the component because we can't reliably calculate it ourselves, sadly.The decorator does a call to
getComponent
which is where the important part of the branded components not including the react-sdk is important: if the branded app includes the react-sdk then the decorator gets executed before the skin has finished loading, leading to all kinds of fun errors. This is also why the skinner lazily loads the react-sdk components to avoid importing them too early, breaking the app.The decorator will end up receiving null for a component because of the getComponent loop mentioned: the require() call is still in progress when the decorator is called, therefore we can't error out. All usages of getComponent() within the app are safe to not need such an error (the return won't be null, and developers shouldn't use getComponent() after this commit anyways).
The AuthPage, being a prominent component, has been converted to demonstrate this working. Changes to riot-web are required to have this work.
The reskindex script has also been altered to reflect these skinning changes - it no longer should set the react-sdk as a parent. The eventual end goal is to get rid of
getComponent()
entirely as it'll be easily replaced by imports.