Allow loading custom application shells. #6605
Conversation
If there's no brackets.app instance try loading a custom shell defined by "appShellImpl" requirejs configuration path. By default, a void implementation for the browser is loaded.
|
Worth clarifying: this is not a PR for Sprint 36. |
ghost
assigned
|
This seems fine but probably needs a much larger architectural discussion. @dangoor any thoughts? |
|
@JeffryBooher OK, I'll try to set that up. |
| "use strict"; | ||
|
|
||
| // this implementation is intentionally left empty | ||
| // see src/main.js for details |
There was a problem hiding this comment.
It seems like this file should actually be named InAppShell or InBracketsShell or some such... running "in browser" (i.e. vanilla Chrome) would actually require filling in a bunch of the APIs here...
There was a problem hiding this comment.
Oh, my idea was that this would be the landing for in-browser shell API implementation, similar to what you've already started in the in-browser branch. This particular comment is misleading. So should I rename the file (would VoidShell do?) or clarify the comment?
There was a problem hiding this comment.
I was assuming that, in the in-browser branch, we'd change the require.config to redefine "appShellImpl" to point to a different impl module. Otherwise I'm confused about why we need that level of indirection: if we're just changing the contents of the same fixed module, we could load it the regular way with no special require.config needed.
If we're keeping the require.config indirection, then I think renaming to something like InNativeShell or InAppShell would be clearer -- since we'd have a different file in the in-browser branch.
|
@dangoor, @JeffryBooher did you come to any conclusion/recommendation? please let me know if this PR should be kept "alive". |
|
@busykai yes, this PR should be fine (subject to the review comments, of course) |
|
@busykai I will follow up with @peterflynn tomorrow about naming. |
|
@JeffryBooher, @peterflynn so how do we call this empty shell? I actually understand @peterflynn's point and can to rename it to InAppShell. |
|
@peterflynn, @JeffryBooher any resolution on this one? please let me know. |
|
@JeffryBooher tagging this medium priority. |
| // https://github.com/adobe/brackets-shell/blob/master/appshell/appshell_extensions.js | ||
| // Change this value to load custom implementation to run brackets in | ||
| // something else than Brackets shell (e.g. in a browser). | ||
| "appShellImpl" : "utils/InBrowserShell" |
There was a problem hiding this comment.
We should probably not put this in utils. Probably best at the root but I could go for a shellImpl folder. This isn't a utility so it doesn't belong there.
|
@busykai just move the file and I think we're good to merge. |
|
aye! |
|
@busykai there is a travis build failure. Can you re-run it to make sure that it's a false-positive? Thanks! |
|
Keeps failing. I will look into it. |
|
@busykai I am going to close this PR. Go ahead and re-open it once you have it building. |
If there's no brackets.app instance try loading a custom shell defined by
"appShellImpl" requirejs configuration path. By default, a void implementation
for the browser is loaded.
It would allows a single configuration point in the similar fashion it's done for the filesystem implementation.
A rudimentary appshell impl could be already found in in-browser branch. Other customers, such as XDK, also need to hack global object creation.
it has no impact on Brackets when ran in Brackets shell.