[DO NOT MERGE] Optimize Webpack Performance and bundle size #7772
Conversation
|
The build is expected to fail because it did not find the react-sdk PR because this PR was opened first. A restart should fix the build failure if the fetch script is smart enough |
|
A small change on react-sdk side is needed before this can get merged |
|
I do think both tests fail because they dont find each other counter branch :( |
MTRNord
changed the title
Tests are failing because it fails to resolve the correct compainion branch. Please test it locally first before merging. |
|
@MTRNord Now that we're past 1.0, I'd like to nudge this forward, but it will need some rebasing. Are you interested in updating it, or shall I take a look? |
|
@jyrans I can try to fix the merge issues. But I expect the CI to still fail because it doesn't find its companion branch on the sdk. You should test it locally before merging! |
I may also re-push it as a branch on the matrix and vector repos once the rebase is ready to hopefully see the green checks there at least. CI can figure out the matching branches, but currently it doesn't work for forked repos like this yet... 😭 |
|
@jryans I just made a rebase on the new develop branch on riot-web (Thats why it says force pushed). A rebase for the sdk is on its way |
jryans
changed the title
|
@MTRNord We just added support for testing matching branches from forks, so your branches here should hopefully be testable now! 😄 If you merge in matrix-org/matrix-react-sdk#2807 and #9212, we should get some proper CI results here. |
|
@jryans will do when near my PC :) |
Took 19 minutes
Took 6 minutes
…" in the package.json to generate correct component-index and overwrite the main resolve of react-sdk to point to the src version of index.js Took 33 minutes
|
@jryans made a rebase to curent develop. Lets see if this still works like on my local PC |
|
Seems like it still doesnt build on CI I will investigate |
|
It did find your matching branches though, so that's great to see! Hopefully the errors that remain are actual problems that can be fixed. |
It seems like they are :) Working on a fix. Seems like it tries to load still from the lib folder on the react sdk somewhere. TBH the error message is not that helpful ^^ |
|
@jryans for some reason it seems to not find the src folder in the react sdk at all. Is it maybe a bug on the resolving itself? |
Some errors are still However there are also which is I guess why you're asking? It's possible this is related to how CI currently checks out the other repos, which is different than what we do on local dev machines: CI checks out the SDKs inside the riot-web directory, which means that are at a different nesting level than a local environment would use. Perhaps you can try simulating this setup locally to see if the errors happen because of it? |
|
@jryans I can do normal build local but not test. So I guess thats a karma issue as well. I am still debugging why it happens. The paths look for node_modules/matrix-react-sdk and as they are on CI linked that path should be fine. |
|
@jryans found the error. I have an alias to make the index.js overwritten. But that one also causes to let imports fail :/ |
Took 8 minutes
…and remove unused imports which broke lint
|
Seems like there is a babel-loader issue too. Will spin up a linux to check in there. It has a different error on my dev PC so it is super hard to debug |
|
Found the error: The import resolving on linux is different on linux than on my windows PC. And the module instead of main usage in package.json causes module.export = {} to not be accepted when importing. I will search for another way |
|
Seems like I essentialy made this a monorepo and babel hates me for doing that. The only way I see is to upgrade babel and babel-loader in riot-web and replace .babelrc with babel.config.js and fix imports in react-sdk to allow importing files that use module.exports :( This might make this PR horrible. Means I will do that, commit it and make a new PR from scratch to clean it up. (This would by the way fix #7731 as a side effect) |
|
This PR is closed. I will reopen until I get this to build again. I guess I will make the babel upgrade a seperate PR too |
MTRNord
changed the title
This commit does what I explained in #7391
It needs this PR too: matrix-org/matrix-react-sdk#2323
Signed-off-by: Marcel Radzio <mtrnord1 [at] gmail.com>
THIS PR MIGHT BREAK OTHER PRS! It is needed that imports of react-sdk reference src instead of lib