-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MultiRoot Workspace support #663
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This PR add a possibility to reboot the reboot device, metro and devtools processes without triggering clean build. It was previously only possible by closing and reopening the IDE panel itself. This also will provide necessary functionality for #663 ### How Has This Been Tested: - run an application and pick "Reboot IDE" option from reload dropdown menu
This PR changes a way we manage build caches. Previous approach was dependent on current vscode workspace, which led to build caches not being accessible when switching from working in the root of the workspace to the root of the application (or doing the same in reverse). The new approach saves this data in global storage, and adds a appRoot identifier to the cache key. This PR is a dependency for #663 ### How Has This Been Tested: - Open a project in the root of the workspace and run some application that is part of it in the IDE. - Open IDE in the root of that application and see if the build cache is loaded. - repeat the proces in reverse - open an application that had a build made before this change and check if migration process works as expected Verify that the build cache migration work: - Build project without these changes - Open the same project with no changes but with the new version of extension – expect the cache hit for native build --------- Co-authored-by: Krzysztof Magiera <[email protected]>
0f8aa95
to
76da6d1
Compare
76da6d1
to
9317a59
Compare
df78b95
to
3dc45d4
Compare
export async function isNodeModulesInstalled( | ||
manager: PackageManagerInfo, | ||
appRoot: string |
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.
Maybe PackageManagerInfo
should hold the appRoot
instead? Isn't the "Package Manager Info" only relevant in the context of a specific "app root"?
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 not sure if it ever gonna be useful in the future, but it is not. You theoretically can call nmp install
in any workspace directory and with the correct setiting all of the will be installed
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.
Yes, but PackageManagerInfo
contains the information on which package manager should be used which, if I understand correctly, depends on the contents of appRoot
, no? Can't changing the app root could cause the selected package manager to change?
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 sometimes could, but it is unlikely, but at the end of the day the important question is not if those values are sometimes related but if there might be the situation ever of changing only one of them and as I have written above there might be
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.
But this is exactly what I mean -- if you change the appRoot
you probably want to recreate the PackageManagerInfo
, right?
The way it's written right now, you supply them separately, so you may provide the "wrong" PackageManagerInfo
, i.e. one unrelated to the passed appRoot
.
packages/vscode-extension/src/webview/providers/LaunchConfigProvider.tsx
Outdated
Show resolved
Hide resolved
) | ||
.then((item) => { | ||
if (item === openLaunchConfigButton) { | ||
commands.executeCommand("workbench.action.debug.configure"); |
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.
Is this the correct action for this? For me it opens the command palette, while I'd expect it to open our launch configuration menu (where I can actually select the app root).
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 don't know why it opens a command palet for you, It works for me and opens a correct configuration file, In the future i would love to migrete to opening our configuration panel but i feel like it is not stable enough yet.
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.
could you run this comand from command palet and see what happens? do you use cursor or vs code?
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.
VSCode.
It's probably because I don't have the file in that project.
This PR refactors how we handle the
appRoot
to allow for changing it in runtime. To make sure that the app root is unified across all the components we now store it inproject.ts
and move all objects dependent onappRoot
to project as well. We listen for launchConfiguration changes and restart everything dependent on appRoot after it changes.We refactor how appRoot is detected:
after analyzing the old approach that was using
workspace.findFiles
we realized that on more elaborate setups it took almost 2 seconds to traverse all of the workspace directories. To combat that we introduce a newfindAppRootCandidates
implementation that takes only a couple of ms in most setups.note: the new function is not perfectly equivalent to the old one as it removes a check if
node_modules
containsreact-native
it is done on purpose as it has a following drawbacks:react-native
in its node modules,node_modules
which are usually quite largenode_modules
to be installed and first time users could encounter problems if we relied on this check.How has dis been tested?
Run the mono repo with 2 application and switch between them.
Run
react-native-svg
repo and select test apps in itRun
react-native-reanimated
repo and select test apps in it, additionally check if cashes are stored properly after switching between applications.To benchmark the
findAppRootCandidates
I added the following code to at the end of theactivate
function in the extension:note: that the benchmark make sens only if you don't have appRoot set in launchConfiguration.
Results for
react-native-reanimated
repository: