Allow starting packager in a sub folder by alsorokin · Pull Request #359 · microsoft/vscode-react-native

alsorokin · 2016-12-01T09:06:31Z

close #312

These changes allow user to specify any folder as a react-native project root in settings.json.

msftclas · 2016-12-01T09:06:34Z

Hi @alsorokin, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Alexander Sorokin (Akvelon)). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

vladimir-kotikov · 2016-12-01T09:13:21Z

+```
+{
+  "react-native-tools": {
+    "projectRoot": "your-react-native-project"


This is a path, relative to workspace's root, right? Could you please make it more clear?

vladimir-kotikov · 2016-12-01T10:43:54Z

+            let settingsPath = path.resolve(vsCodeRoot, ".vscode/settings.json");
+            let settingsContent = fs.readFileSync(settingsPath, "utf8");
+            // strip the json off of comments
+            settingsContent = settingsContent.replace(/\/\/.*\n/g, "");


You might want to use strip-json-comments to make it parseable

vladimir-kotikov · 2016-12-01T10:45:54Z

+            settingsContent = settingsContent.replace(/\/\/.*\n/g, "");
+            let parsedSettings = JSON.parse(settingsContent);
+            let projectRootPath = parsedSettings["react-native-tools"].projectRoot;
+            if (path.isAbsolute(projectRootPath)) {


You don't need to check if path is absolute - path.resolve(vsCodeRoot, projectRootPath) would yield back projectRootPath if it's absolute, despite of what is in vsCodeRoot

vladimir-kotikov · 2016-12-01T12:08:11Z

     */
    private requestSetup(debugSession: NodeDebugSession, args: any) {
-        this.projectRootPath = path.resolve(args.program, "../..");
+        this.setProjectRoot(args);


Nit: If the only purpose of setProjectRoot is to set this.projectRootPath, IMO it'd be more transparent and readable to make a function that would return projectRootPath, and use it's result.

MSLaguana · 2016-12-01T17:28:39Z

 import * as path from "path";
 import * as Q from "q";
 import * as XDL from "./xdlInterface";
+import * as vscode from "vscode";


You can't refer to the vscode package from this file as-is: Sources under src/common can be referenced both from within the vscode extension process and within the external debug adapter process, and the debug adapter process cannot access vscode.

MSLaguana · 2016-12-01T17:30:43Z

 import * as path from "path";
 import * as XDL from "../common/exponent/xdlInterface";
 import * as url from "url";
+import * as vscode from "vscode";


As above, we can't directly refer to vscode in here since it is also accessed from within the separate debug adapter process.

MSLaguana · 2016-12-05T16:24:55Z

-     * {operation} - a function that performs the expected operation
     */
    public isReactNativeProject(): Q.Promise<boolean> {
+        if (!fs.existsSync(path.join(this.workspaceRoot, "package.json"))) {


Is this the right condition? I believe that running react-native foo requires you to be in the folder with the node_modules/react-native folder inside it, so if your folder structure looks something like

root root/project/package.json root/project/node_modules/react-native root/site/<other content>

then workspaceRoot will point to root, but the react-native project is located at root/project and that's where the package.json must be, and where we should start react-native from.

This workspaceRoot variable actually is pointing at project root. I'll rename it. Thanks for noticing this inconsistency!

MSLaguana · 2016-12-05T16:29:43Z

+    private getProjectRoot(args: any): string {
+        try {
+            let vsCodeRoot = path.resolve(args.program, "../..");
+            let settingsPath = path.resolve(vsCodeRoot, ".vscode/settings.json");


This won't be the default location where vscode stores workspace settings: If you have the workspace open in root and the project in root/project, then this path I think would be rooot/project/.vscode/settings.json, while opening up the local workspace settings would open up root/.vscode/settings.json. Unless I'm misinterpreting where you intend to place the entry point that we run?

Well yes, I guess the entry point is still in root/.vscode/
If that is not the right place for it to be and we put it to root/project/.vscode, then the user would need to manually edit program arg in launch.json.
Currently it looks like this:

{ <...> "configurations": [ { ... "program": "${workspaceRoot}/.vscode/launchReactNative.js", ... }] <...> }

And as far as I know we can't really use any other variable other than $workspaceRoot here so we don't have any convenient way to point to project root.

You're right, I forgot that we have to specify the path in launch.json. Looking at https://code.visualstudio.com/docs/editor/tasks#_variable-substitution it seems that the variables which can be substituted are limited, but it does include environment variables, so we could do something a bit hacky and have the extension set a VSCODE_REACT_NATIVE_PROJECT_ROOT environment variable and reference that, but I don't like that idea much.

Although, looking at the vscode codebase I do see a test with https://github.com/Microsoft/vscode/blob/ea26b957f6c593410724cd341e1db2c7c37170b8/src/vs/workbench/services/configurationResolver/test/node/configurationResolverService.test.ts#L117 which might suggest that we could reference ${config['react-native-tools'].projectRoot} or something and it might work out?

If not, I think the current approach is ok

Tried it and it doesn't seem to work 😞

close #312

MSLaguana · 2016-12-06T17:40:53Z

    AppRegistry.runApplication('${componentName}', appParameters);
 });`;
-                return this.fileSystem.writeFile(this.dotvscodePath(EXPONENT_INDEX), fileContents);
+                return this.fileSystem.writeFile(this.pathToFileInWorkspace(EXPONENT_INDEX), fileContents);


I'm not sure that we want to put this file in with the user's sources rather than in the .vscode folder; it's not really part of the end-user's app, it's just an implementation detail of us converting to an exponent app.

MSLaguana · 2016-12-06T17:43:48Z

                            }
                            if (runAs === PackagerRunAs.EXPONENT) {
-                                args = args.concat(["--root", ".vscode"]);
+                                args = args.concat(["--root", path.relative(this.projectPath, path.resolve(this.workspacePath, ".vscode"))]);


This path needs to point to wherever the exponentIndex.js file is I believe, otherwise the exponent packager won't find the entry point (or the rest of the app)

I've moved the exponentIndex.js file back to root/.vscode folder as you suggested, so this path should be correct.
I've tested it with Exponent and it works.
See f6b41bb

close #312

MSLaguana · 2016-12-07T22:50:15Z

Thanks @alsorokin!

msftclas added the cla-not-required label Dec 1, 2016

vladimir-kotikov reviewed Dec 1, 2016

View reviewed changes

vladimir-kotikov approved these changes Dec 1, 2016

View reviewed changes

MSLaguana reviewed Dec 1, 2016

View reviewed changes

MSLaguana reviewed Dec 5, 2016

View reviewed changes

alsorokin and others added 2 commits December 6, 2016 09:26

Allow starting packager in a sub folder

38edb09

close #312

Merge branch 'master' into packager-subfolder

ba3d9b8

MSLaguana reviewed Dec 6, 2016

View reviewed changes

Allow starting packager in a sub folder

f6b41bb

close #312

MSLaguana merged commit 23c027b into microsoft:master Dec 7, 2016

alsorokin deleted the packager-subfolder branch February 15, 2017 15:09

vladimir-kotikov mentioned this pull request Jul 25, 2017

Expo support upgrade #478

Merged

This was referenced Jun 17, 2026

Bump js-yaml override to 4.2.0 (Dependabot #359) #2729

Open

Bump js-yaml override to 4.2.0 #2730

Open

Conversation

alsorokin commented Dec 1, 2016

Uh oh!

msftclas commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alsorokin Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSLaguana commented Dec 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alsorokin Dec 6, 2016 •

edited

Loading