1124326 - Support Cordova projects in WebIDE

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

With pre-push / build step support (bug 1000993), we are very close to support Cordova projects.

We just need a few tweaks due to the manifest only being present in build output dir, instead of the root dir.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Build more often to support Cordova. r=ochameau

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8552791 [details] [diff] [review]
Build more often to support Cordova. r=ochameau

Review of attachment 8552791 [details] [diff] [review]:
-----------------------------------------------------------------

Alex, I am now building more often to support Cordova.

How does this feel with a Gaia app?  Is it too slow now?  I could investigate building in the background if so.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8552791 [details] [diff] [review]
Build more often to support Cordova. r=ochameau

Review of attachment 8552791 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix env issue.
Also, that would be great if we can live without calling build step on project change.

Otherwise it looks good to build when adding/importing an app.

::: browser/devtools/webide/modules/app-manager.js
@@ -304,5 @@
> -      }
> -
> -      this.update("project");
> -
> -      this.checkIfProjectIsRunning();

The two new calls to build step in import/new shouldn't be painful,
but I have some doubts about this one.
When you have a gaia app, the build step is quite CPU consuming and takes quite some time and it seems to delay UI switching to the selected gaia app. I imagine it allows to update more frequently data coming from build step, but if the build step is slow, we have to be careful about when we call it and limit the calls!

In a very naive workflow, we restart firefox, [auto-]select the app, rebuild it, want to update it to the phone, rebuild it a second time for nothing, push it to the phone.

::: browser/devtools/webide/modules/build.js
@@ +58,5 @@
>  
>        args = manifest.prepackage.args || [];
>        args = args.map(a => a.replace(/%project%/g, project.location));
>  
>        env = manifest.prepackage.env || [];

Please ensure mergin `let env = []` set couple of lines before and manifest.prepackage.env ! Otherwise it is breaking gaia support.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c56b4530c94b

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: 0001-Bug-1124326-Improve-packageDir-support-for-Cordova.-.patch

This version no longer builds on project switch.  It turns out all we needed was to know where the packageDir lives, so that's now separate from actually building.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: 0002-Bug-1124326-Support-Cordova-w-o-build-file.-r-ochame.patch

For Cordova, let's fake out the package.json file so you can build any Cordova project as-is.

If people want to use a more complex command, then they can still add a package.json to do so.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Fixed test failure.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8aa5ac99a56a

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: 0001-Bug-1124326-Improve-packageDir-support-for-Cordova.-.patch

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8566062 [details] [diff] [review]
0001-Bug-1124326-Improve-packageDir-support-for-Cordova.-.patch

Review of attachment 8566062 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I just had two nits.

::: browser/devtools/webide/modules/app-manager.js
@@ +293,5 @@
>  
>        // Clear out tab store's selected state, if any
>        this.tabStore.selectedTab = null;
>  
> +      this._switchProjects(this._selectedProject).then(() => {

Shouldn't we use this.selectedProject?

@@ +324,5 @@
> +    }
> +    if (project.type == "tab") {
> +      this.tabStore.selectedTab = project.app;
> +    }
> +  }),

nit: I don't see a big value in pulling that off from selectedProject getter, except that we do wait for validateProject completion, was that important?

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> ::: browser/devtools/webide/modules/app-manager.js
> @@ +293,5 @@
> >  
> >        // Clear out tab store's selected state, if any
> >        this.tabStore.selectedTab = null;
> >  
> > +      this._switchProjects(this._selectedProject).then(() => {
> 
> Shouldn't we use this.selectedProject?

Since we are inside the setter for |selectedProject|, I think it's fine to use the raw value, since you're at the point where the raw value is set.

> @@ +324,5 @@
> > +    }
> > +    if (project.type == "tab") {
> > +      this.tabStore.selectedTab = project.app;
> > +    }
> > +  }),
> 
> nit: I don't see a big value in pulling that off from selectedProject
> getter, except that we do wait for validateProject completion, was that
> important?

Fair enough, I'll merge it back in.  I think the yield was needing in a previous version, but it seems okay without it now.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/7d7b31036bb7
remote:   https://hg.mozilla.org/integration/fx-team/rev/48d76f7edcfe

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7d7b31036bb7
https://hg.mozilla.org/mozilla-central/rev/48d76f7edcfe

Comment 13

[Will Bamberg [:wbamberg]]

Here's what I've done for this:

* new page: https://developer.mozilla.org/en-US/docs/Tools/WebIDE/Working_with_Cordova_apps_in_WebIDE

* new section in the Apps docs for Cordova support, linking to the new page: https://developer.mozilla.org/en-US/Apps/Tools_and_frameworks/Cordova_support_for_Firefox_OS#Testing_and_debugging_using_WebIDE

* added a note in "Custom build step" https://developer.mozilla.org/en-US/docs/Tools/WebIDE/Running_and_debugging_apps#Running_a_custom_build_step saying that the Cordova package.json is no auto-generated

Let me know what you think.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Will Bamberg [:wbamberg] from comment #13)
> Let me know what you think.

Cool, this looks great to me!