| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Landing CDT-independent livedev implementation #10010
Conversation
Brings in new LiveDevelopment implementation incubated at njx/brackets-livedev2 which allows running on multiple browsers.
|
Well, it seems that Travis is finally happy after those 3 commits. I realized that have JSLint set instead of JSHint. Please, let me know if they added some noise, I can create a new PR from a clean branch if that helps reviewing it. Thanks! |
|
We'd appreciate if this was given a priority to land it ASAP -- we're using this in our production and we're being left without a space to develop it further (logistically, this deprecates the extension version njx/brackets-livedev2, but it's still not in core). We cannot afford to have this hanging in limbo for a long time. I don't mean taking shortcuts, of course, just a high priority. Thanks! |
|
@redmunds, sounds great! next week works just fine. |
| exports._getCurrentLiveDoc = _getCurrentLiveDoc; | ||
| exports._getInitialDocFromCurrent = _getInitialDocFromCurrent; | ||
|
|
||
| // Export public functions |
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.
LiveDevMultiBrowser and LiveDevelopment are inter-changeable implementations, so the public APIs must be the same. Currently, there are a lot more exports for LiveDevelopment. Some of those (e.g. LiveDevelopment.agents) can maybe be moved to unit testing section, but the rest should at least be defined and stubbed out.
|
@redmunds, I've already included the missing methods to align |
|
I'm seeing 3 Integration Code Inspection tests fail on Windows -- probably just need to sync with latest master. |
|
Running the Live Preview: MultiBrowser (experimental) - Live Development tests, they pass on Win7/Firefox, but I'm getting failures (different number every time: 3 or more) on MacOS 10.8/Safari 6.2. |
|
@sebaslv, I'll look into the tests on OSX/Safari combination. |
| // sets the MultiBrowserLiveDev implementation if multibrowser = true, | ||
| // keeps default LiveDevelopment implementation based on CDT in other case. | ||
| // since UI status are slightly different btw implementations, it also set | ||
| // the corresponding style values. |
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.
Instead of livedev.multibrowser preference being a boolean, what do you think about making it an Object?
It would have to define:
- path to module
- status tooltip array
- status style array
This way, anyone could define their own Live Preview implementation. All of the LiveDevMultiBrowser code could stay as an default/extension to illustrate how to add new implementations.
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.
Thinking about this further, we'd need some way to "register live preview provider".
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.
To make this change (in a clean way) I think I would need to change LiveDevelopment/main.js to dynamically load modules and move all the specifics things of the current implementation from main.js to LiveDevelopment module probably (eg. Inspector initialization). Not sure if that could introduce some issues with the current implementation but also with extensions that depends on it.
Making status/tooltips as a preference would probably make every developer have to know and write their values every time they want to switch among impls. The new implementation was aligned to the current UI just to make simpler its integration but, I think it is not its natural UI. It could have multiple instances running on different browsers which should be monitored/controlled somehow, even the concept of live preview session and its states would probably change. Not sure about this, I don't like current solution either, it could eventually be moved to some a config file or behind each module implementation.
Moving the new implementation back to be an extension and make it switchable with the core implementation is something I didn't try before. I assumed not to be an extension because we thought it would replace the current implementation in the long term and provide mechanisms for extending it by building custom transports, launchers, protocol methods, remote functions, etc. I didn't take into account building custom implementations because it seemed to me that building an abstraction layer on top of the current LiveDevelopment wouldn't be easy.
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 brought this up at team meeting today and consensus is that it's ok as it is for now.
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 think we have a use case for multiple providers.. this is more of a "land behind a feature flag" kind of thing.
|
Done with code review. |
|
I also tried out this branch with brackets-shell CEF 2171 and everything looks good! |
- align documentation for _setTransport - restore object definitions broken in the previous commit
|
I restored previous definitions and also changed doc for |
|
@sebaslv This looks good to me. I still see the |
|
@randy, it is ready. I will take off the |
busykai
changed the title
|
Thanks for all of the work on this! Merging. |
Landing CDT-independent livedev implementation
| // since UI status are slightly different btw implementations, it also set | ||
| // the corresponding style values. | ||
| function _setImplementation(multibrowser) { | ||
| console.log('set implementation ' + multibrowser); |
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've just tried this the first time and well, I'm quite impressed. I wouldn't have thought this is possible and admittedly, I don't have any idea how this is implemented :) đź‘Ť |
|
@marcelgerber, yeah the message logging will be eventually turned off. However, as it is still experimental, these are needed for the developers of the feature itself. |
Brings in new Live Preview implementation incubated at njx/brackets-livedev2 which allows running on multiple browsers (more details on the internals at
README.md). The new implementation can be used as a swap-in replacement of the currentLiveDevelopmentimplementation.Set
livedev.multibrowserpreference to true in order to make it work. If enabled, it will launch the page in your default browser when clicking on the Live Preview icon as it works today. You should then also be able to copy and paste the URL from that browser into any other browser, live edits will then update all connected browsers at once.Main assets will land under
LiveDevelopment/MultiBrowserImplwhile the main moduleLiveDevMultiBrowser(equivalent toLiveDevelopment) will land underLiveDevelopmentfolder. This is probably not the more elegant choice but the decision mainly relies on these two needs:LiveDevelopmentwithout moving them from the original place since many extensions require them as dependenciesLiveDevelopment.Changes included in
LiveDevelopment/main.jsallows managing both implementations. Both modules are loaded and initialized. A listener for preference changes is in charge of setting the active implementation based on pref value. Current implementation will be active and work as today by default.There are still key things to do (launchers for particular browsers, UX/UI, multi-host support, unit/integration tests that run on different browsers, among others). Some of them will also need some previous discussion but, it is important to get this implementation landed in core so we can get some feedback from developers and also submit PRs that allows evolving it including changes that could potentially impact some areas of the underlying infrastructure.