Support (ios)-project-path param #227

mroswald · 2016-04-20T21:39:20Z

I need your help please :-) I wanted to support the new --project-path param for "react-native run-ios" which allows you to have your iOS project in a subfolder (like Examples/MyExampleApp/ios/).

Currently I'm struggling with the VSC not matching the source code files to the source maps. In the ScriptImporter I change the "/", which represent the paths, to "_". But the IDE itself does not recognize / map it to the source files. I can't find where this happens - can you enlighten me? :-)

This PR is related to facebook/react-native#6134

msftclas · 2016-04-20T21:39:24Z

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

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

mroswald · 2016-04-20T21:44:14Z

Just a short question aside: Which part does control the order in which the launch arguments are coming in? It's just a mess that the (vsc) extension relies on the correct order and count of parameters. Is there no better way to get them?

MSLaguana · 2016-04-20T21:44:23Z

Our extension builds on top of the core vscode node debugging extension; you can see their code at https://github.com/Microsoft/vscode-node-debug

We primarily try to make sure that the project fits their requirements, and then pass it over. Why are you modifying the path at all?

MSLaguana · 2016-04-20T21:50:03Z

Regarding your second question: The current architecture is a little bit of a mess, but we haven't had a chance to clean it up yet.

There are 2 parts to the debugger: one is the nodeDebugWrapper which is a proper vscode debug adapter, and in its launchRequest function it is given the data from the appropriate launch.json entry.

However we have implemented most of our logic in the second part of the debugger, which is actually in what is normally the app being debugged. This is everything starting from launchReactNative.js. Since this isn't a standard way of doing things, we went with the path of least resistance and pass the few properties we need via the argv of the process. The argv is read in launcher.ts, and constructed in nodeDebugWrapper.ts.

Ideally the bulk of our logic should be stripped out of the "app being debugged" process and put into the debug adapter process, but as I said we haven't had time to go back and fix it yet.

digeff · 2016-04-20T22:16:17Z

src/debugger/nodeDebugWrapper.ts

@@ -49,6 +49,7 @@ interface ILaunchArgs {
    platform: string;
    target?: string;
    internalDebuggerPort?: any;
+    iosProjectPath?: string;


In the react-native help it says that this path is relative the the project root. I think it'd be clearer if we renamed this argument to iosRelativeProjectPath, and then in iOSPlatform.ts we can combine it with projectPath to create the value iosProjectPath.

guillaumejenkins · 2016-04-20T22:25:32Z

There were a few tslint errors which caused the automated build to fail, reporting them here for convenience:

[gulp-tslint] error (no-trailing-whitespace) src/debugger/nodeDebugWrapper.ts[164, 1]: trailing whitespace
[gulp-tslint] error (no-trailing-whitespace) src/debugger/nodeDebugWrapper.ts[168, 1]: trailing whitespace
[gulp-tslint] error (quotemark) src/debugger/scriptImporter.ts[75, 99]: ' should be "
[gulp-tslint] error (quotemark) src/debugger/scriptImporter.ts[86, 118]: ' should be "

digeff · 2016-04-20T22:46:54Z

src/common/ios/xcodeproj.ts

@@ -20,6 +20,7 @@ export class Xcodeproj {
    }

    public findXcodeprojFile(projectRoot: string): Q.Promise<string> {


I think you should pass the absolute iosProjectPath to this method instead. I think that most ios methods only need to know the location of the ios project, so you'll be able to remove the projectRoot in most of them if you pass the iosProjectPath instead.

I do, this line only checks if there is a trailing "ios/" and removes it.

All the places where you are using that regexp are iOS related code, and they are all using projectRoot + "ios" in every case except for calls to new CommandExecutor(this.projectRoot) which I don't think it'll break if you also change it to use projectRoot + "ios" instead.

I think it'd be better to just pass iosProjectRoot to:
PlistBuddy.getBundleId
Xcodeproj.findXcodeprojFile
new Compiler
new DeviceDeployer
new DeviceRunner

And then modify the code where they later add the "ios" to the path, so the code still keeps working (as in this line: https://github.com/mroswald/vscode-react-native/blob/feat/support-iosprojectpath-param/src/common/ios/plistBuddy.ts#L29 )

mroswald · 2016-04-21T08:26:12Z

@guillaumejenkins thanks. I didn't care about linting since the functionality is not complete :-) I'll fix the issues soon

mroswald · 2016-04-21T08:36:51Z

@MSLaguana thanks for your in-deep explanation. The reason why I want to change this, is that some React Native projects do not have a single "ios" folder, but a subfolder structure where one or more XCode projects live. The React Native cli got this feature with 0.24 (facebook/react-native#6134).

digeff · 2016-04-21T17:30:36Z

@mroswald I think that @MSLaguana Was asking why are you replacing the / for _ in the paths in src/debugger/scriptImporter.ts ?

Adding support for --project-path completely makes sense!

mroswald · 2016-04-21T20:04:33Z

@digeff @MSLaguana ah, sry :-) That's the point on which I'm struggling. In .vscode/.react/ there is no directory structure, so I'd have to create it. But then vs code does not map / finds the connection between source code and downloaded source + sourcemap. I'm not sure how this is supposed to work

digeff · 2016-04-21T20:07:09Z

In the cases we've seen so far, only the bundle file is downloaded. What's your scenario? Why do you need a directory structure there? Which scripts are being imported in your project?

mroswald · 2016-04-21T20:28:10Z

Only index.ios.bundle and index.ios.map. I also tried to remove the path and just save them directly into this directory. That works so far, but the breakpoints are moved (when starting the debugging) from line x to something like line 32034 which is near the "breakpointed" module in the bundle (index.ios.js). Not sure what is responsible for matching/moving the breakpoints

digeff · 2016-04-21T20:31:05Z

We have some known issues with matching breakpoins: #74 #206
Any chance you are running into those issues?

mroswald · 2016-04-21T20:33:48Z

Looks not that like these. What actually works is to set the breakpoint via "debugger;" statement. But then it stops inside the index.ios.bundle.

digeff · 2016-04-21T20:35:17Z

You can add the launch option:
"trace": "sm"
To get some source maps debugging information printed on the Debug Console. That might help you figure out what's happening.

mroswald · 2016-04-21T20:48:22Z

Ok, this is what the trace tells me:

40513: _mapSourceAndUpdateBreakpoints: src: '/Users/mark/**/Examples/**/index.ios.js' 20:0 -> gen: '/Users/mark/**/.vscode/.react/index.ios.bundle' 37025:0
40513: _mapSourceAndUpdateBreakpoints: src: '/Users/mark/**/Examples/**/index.ios.js' 32:0 -> gen: '/Users/mark/**/.vscode/.react/index.ios.bundle' 37037:0

(I had to hide the project names here. /Users/mark/** is my project root, Examples is the directory which hosts the ios Projects)

digeff · 2016-04-21T20:53:02Z

That means that the source map it's telling VS Code that the line index.ios.js:20:0 matches to: index.ios.bundle:37025:0
If that's not true, it's probably due to this bug: facebook/react-native#6946 .
If that mapping is actually correct, then you may have found a bug in either the Node Debugger or VS Code itself.

mroswald · 2016-04-21T22:10:08Z

Ok, so then this is view is telling me only the mapping?

Second: What does this _getStackFrame mean?

41100: _mapSourceAndUpdateBreakpoints: src: '/Users/mark/**/Examples/**/index.ios.js' 20:0 -> gen: '/Users/mark/**/.vscode/.react/index.ios.bundle' 37025:0
41100: _getStackFrame: gen: '/Users/mark/**/.vscode/.react/index.ios.bundle' 37037:4 -> src: '/Users/mark/**/Examples/**/index.ios.js' 32:0
41100: _getStackFrame: gen: '/Users/mark/**/.vscode/.react/index.ios.bundle' 37044:1 -> src: '/Users/mark/**/Examples/**/index.ios.js' 39:0

index.ios.js has 35 lines in total.

btw: thanks for this overwhelming support!

digeff · 2016-04-21T22:12:20Z

Yes, that's the bug with the react-native source maps not mapping to the correct line numbers. We are hoping that react-native will fix this issue soon,

mroswald · 2016-04-21T22:17:42Z

Ok, I just tested a breakpoint in a file without JSX / without a react component. It's definitely related to transpiled jsx or react classes. Let me just clean my current code, then I'll push an update.

mroswald · 2016-04-22T10:15:01Z

@digeff Ok, I refactored as you suggested (relative path)

digeff · 2016-04-22T16:27:06Z

src/debugger/sourceMap.ts

@@ -15,7 +15,7 @@ interface ISourceMap {
 }

 export class SourceMapUtil {
-    private static SourceMapURLRegex: RegExp = /\/\/(#|@) sourceMappingURL=(.+?)\s*$/m;
+    private static SourceMapURLRegex: RegExp = /\/\/(#|@) sourceMappingURL=(.+index\.ios\.map.*?)\s*$/m;


We've seen some example projects where this file is not called index.ios. Also, this won't work for Android. Why is this change necessary?

In my example, there are different SourceMaps provided. But you're right - Shall we change that to index\.(android|ios)\.map?

What's the advantage of that implementation over the current implementation?

If I remember correctly, in the Movies example app, this file is called Movies.ios.map, so that change will break that example.

If the script body returns multiple sourceMappingUrl= then this error happens:

[6:39:12 PM] <END> request:/**/index.ios.bundle?platform=ios (67ms) [6:39:12 PM] <START> request:/exp.js.map Error: Error validating module options: child "platform" fails because ["platform" must be a string] at declareOpts.js:60:13 at index.js:222:20 at tryCallOne (/Users/mark/**/node_modules/promise/lib/core.js:37:12) at /Users/mark/**/node_modules/promise/lib/core.js:123:15 at flush (/Users/mark/**/node_modules/asap/raw.js:50:29) at _combinedTickCallback (node.js:370:9) at process._tickCallback (node.js:401:11)

$ grep "sourceMappingURL=" index.ios.bundle //# sourceMappingURL=exp.js.map //# sourceMappingURL=cldr.js.map //# sourceMappingURL=core.js.map //# sourceMappingURL=en.js.map //# sourceMappingURL=main.js.map //# sourceMappingURL=utils.js.map //# sourceMappingURL=es5.js.map //# sourceMappingURL=compiler.js.map //# sourceMappingURL=en.js.map //# sourceMappingURL=main.js.map //# sourceMappingURL=diff.js.map //# sourceMappingURL=es5.js.map //# sourceMappingURL=core.js.map //# sourceMappingURL=es5.js.map //# sourceMappingURL=memoizer.js.map //# sourceMappingURL=core.js.map //# sourceMappingURL=parser.js.map //# sourceMappingURL=index.ios.map

The only source map of interest is the index.ios/android.map, right? Or should we trust that the correct one is the last match?

If the source has multiple sourceMappingURL entries, that is a violation of the spec and the source must be cleaned up (There is a babel loader called something like strip-sourcemap which does this I believe). If we want to try and support non-compliant pages, then my suggestion would be picking the last sourceMappingURL, not fuzzy matching on the assumed name.

Ok, you're right. I'll investigate why we have multiple source maps in one body (maybe it's related to babel telling me [BABEL] Note: The code generator has deoptimised the styling of "/Users/mark/**/node_modules/immutable/dist/immutable.js" as it exceeds the max of "100KB". and remove this change from the PR

Ok, found it. This is an issues related to react-intl

formatjs/formatjs#71

msftclas · 2016-04-22T18:40:51Z

@mroswald, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

mroswald · 2016-04-25T14:34:45Z

@digeff anything left?

digeff · 2016-04-25T17:35:15Z

package.json

+                            "iosRelativeProjectPath": {
+                                "type": "string",
+                                "description": "Relative path to the ios/ folder, if it is not located on the project root.",
+                                "default": null


The default in this case would be "ios" (That what we'll use if they don't provide this property).

- always send --project-path param to react-packager - removed change to sourceMap pattern - allow empty string for iosRelativeProjectPath

mroswald · 2016-04-25T20:59:26Z

@digeff I rebased to current master, tests should pass now (fingerscrossed)

digeff · 2016-04-25T22:09:20Z

@mroswald Have you tested this both on iOS simulators and a real iOS device?

mroswald · 2016-04-25T23:19:50Z

@digeff sorry, forgot one. Tested on simulator and iPhone with one project having ios in root folder and one having xcode project in subfolder

digeff · 2016-04-25T23:21:07Z

src/common/ios/xcodeproj.ts

@@ -21,7 +21,7 @@ export class Xcodeproj {

    public findXcodeprojFile(projectRoot: string): Q.Promise<string> {
        return this.nodeFileSystem
-            .findFilesByExtension(path.join(projectRoot, "ios"), "xcodeproj")
+            .findFilesByExtension(path.join(projectRoot), "xcodeproj")


You can remove the call to path.join

right, that can be removed

digeff · 2016-04-25T23:49:02Z

👍

MSLaguana · 2016-04-26T21:51:47Z

src/debugger/scriptImporter.ts

@@ -71,7 +71,7 @@ export class ScriptImporter {
     * Writes the script file to the project temporary location.
     */
    private writeAppScript(scriptBody: string, scriptUrl: url.Url): Q.Promise<String> {
-        let scriptFilePath = path.join(this.sourcesStoragePath, scriptUrl.pathname); // scriptFilePath = "$TMPDIR/index.ios.bundle"
+        let scriptFilePath = path.join(this.sourcesStoragePath, path.basename(scriptUrl.pathname)); // scriptFilePath = "$TMPDIR/index.ios.bundle"


Just to make sure I understand correctly, these two path.basename changes are so if the packager gets a request for http://localhost/foo/index.bundle.ios it will search in .vscode/.react/index.bundle.ios and not .vscode/.react/foo/index.bundle.ios, is that right?

If that is correct, then while this wouldn't work for multiple apps running concurrently, it should work for running one app at a time I think, so it looks ok to me.

@MSLaguana I hate this timezone difference :-) The React Native packager can serve multiple apps at a time, but vscode won't hande multiple apps concurrently (as all index.ios* maps will be stored at .vscode/.react/index.bundle.ios. I experimented with replacing the path separator /with _ but then the IDE didn't match anything. That's the part where I don't have a clue how that works. How does vscode map the source code to anything stored under .vscode/.react?

But - anyway. Just for the moment this will be an improvement (@digeff merge pls :-) ). I'm willing to drive this to the next level - in another PR, hopefully with your assistance.

The source files refer to a sourcemap at the end of the file, and we do some tweaks to that (see https://github.com/Microsoft/vscode-react-native/blob/master/src/debugger/sourceMap.ts#L70 ) to make sure that it refers to the correct file. As long as the foo.js file refers to a bar.js.map file in a sourcemap comment, I believe that the debugger should correctly pick up the source map.

digeff · 2016-04-26T23:06:46Z

@mroswald Thank you very much for your contribution!

The code that we download to .vscode/.react/ gets executed in the appWorker.ts file on line 116:
vm.runInContext(contents, this.sandboxContext, filename);

The issues you may run into source mapping if you modify the file path, is that the bundle file has a reference to the .map.js file, and the .map.js file has references to the bundle and to the source files themselves. You need to make sure that all the paths are valid if you change anything. Maybe it'd be easier to just download foo/index.ios.bundle to .vscode/.react/foo/index.ios.bundle

Does the packager support debugging multiple apps at the same time? Does this work in Chrome?

mroswald · 2016-05-27T17:12:32Z

@digeff Is it possible to release a new version? There are bugs left on the milestone, but indeed the features are not usable until this bugs are fixed.

digeff · 2016-05-27T17:19:34Z

@mroswald We are looking into creating a new release in a week or so

msftclas added the cla-required label Apr 20, 2016

mroswald force-pushed the feat/support-iosprojectpath-param branch from d949071 to fbcf9a0 Compare April 20, 2016 21:40

digeff reviewed Apr 20, 2016
View reviewed changes

digeff reviewed Apr 22, 2016
View reviewed changes

msftclas added the cla-signed label Apr 22, 2016

msftclas removed the cla-required label Apr 22, 2016

digeff reviewed Apr 25, 2016
View reviewed changes

mroswald added 9 commits April 25, 2016 22:51

Allow (ios) --project-path configuration for starting the app

398e2b0

fix script import paths and change order of launch arguments

c96fc63

refactor ios project path parameter

bb77358

simplify getBundleId call

5c32e64

slight improvements

d235274

- always send --project-path param to react-packager - removed change to sourceMap pattern - allow empty string for iosRelativeProjectPath

remove unnecessary check

a255055

add iosRelativeProjectPath param to package.json

d8196bd

remove adding "ios" to path, change default value, cleanup

b7773a6

rebased and fixed IRunOptions for android compability

eef7ed0

mroswald force-pushed the feat/support-iosprojectpath-param branch from 32432d6 to eef7ed0 Compare April 25, 2016 20:58

remove ios from path, should be passed in

cc575e6

digeff reviewed Apr 25, 2016
View reviewed changes

remove path.join

1d68952

Merge branch 'master' into feat/support-iosprojectpath-param

3806430

MSLaguana reviewed Apr 26, 2016
View reviewed changes

digeff merged commit 7091dc1 into microsoft:master Apr 26, 2016

		@@ -20,6 +20,7 @@ export class Xcodeproj {
		}

		public findXcodeprojFile(projectRoot: string): Q.Promise<string> {

Support (ios)-project-path param #227

Support (ios)-project-path param #227

Conversation

mroswald commented Apr 20, 2016 • edited

msftclas commented Apr 20, 2016

mroswald commented Apr 20, 2016

MSLaguana commented Apr 20, 2016

MSLaguana commented Apr 20, 2016

Choose a reason for hiding this comment

guillaumejenkins commented Apr 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroswald commented Apr 21, 2016

mroswald commented Apr 21, 2016

digeff commented Apr 21, 2016

mroswald commented Apr 21, 2016

digeff commented Apr 21, 2016

mroswald commented Apr 21, 2016

digeff commented Apr 21, 2016

mroswald commented Apr 21, 2016

digeff commented Apr 21, 2016

mroswald commented Apr 21, 2016 • edited

digeff commented Apr 21, 2016

mroswald commented Apr 21, 2016 • edited

digeff commented Apr 21, 2016

mroswald commented Apr 21, 2016

mroswald commented Apr 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroswald Apr 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msftclas commented Apr 22, 2016

mroswald commented Apr 25, 2016

Choose a reason for hiding this comment

mroswald commented Apr 25, 2016

digeff commented Apr 25, 2016

mroswald commented Apr 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digeff commented Apr 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digeff commented Apr 26, 2016

mroswald commented May 27, 2016

digeff commented May 27, 2016

mroswald commented Apr 20, 2016 •

edited

guillaumejenkins commented Apr 20, 2016 •

edited

mroswald commented Apr 21, 2016 •

edited

mroswald commented Apr 21, 2016 •

edited

mroswald Apr 22, 2016 •

edited