Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate frontend #9915

Merged
merged 14 commits into from Oct 25, 2017
Merged

Consolidate frontend #9915

merged 14 commits into from Oct 25, 2017

Conversation

balloob
Copy link
Member

@balloob balloob commented Oct 17, 2017

Description:

This moves all frontend related static assets to the polymer repo. Building scripts will also be moved to the repo. We will no longer contain a version of the frontend ready to go when doing a git checkout. Frontend will still be included in the PyPi package.

This PR is the first step. Probably some more cleanup PRs will follow after this.

Polymer PR: home-assistant/frontend#468

Breaking change:

Developers only: frontend has been refactored. The method register_panel has been turned into a coroutine function called async_register_panel. The parameter url_path has been renamed to frontend_url_path. For frontend, development, you no longer pass development: 1 to the http component but instead configure the frontend component to be in development mode by pointing it at a local checkout of the Polymer repo:

frontend:
  development_repo: ../home-assistant-polymer

@andrey-git
Copy link
Contributor

Will pip3 install -e . used in script/setup still work?

I'm thinking of a developer who just wants to develop backend:
script/setup will make hass point to the git-checked-out-version, but that version now doesn't have FE files.

@balloob
Copy link
Member Author

balloob commented Oct 18, 2017

No. One will no longer be able to work on backend stuff without compiling the frontend once.

To make it easier to compile I'll include a Dockerfile. In a future PR we could add a script that takes the latest release from PyPi and extracts the frontend.

@balloob
Copy link
Member Author

balloob commented Oct 18, 2017

We're not going to merge these PRs before 0.56 release.

@andrey-git
Copy link
Contributor

Will one be able to work on backend stuff if they don't have frontend: in their config?

I thought the intent of this PR was to decouple FE from the BE?

@balloob
Copy link
Member Author

balloob commented Oct 18, 2017

One can always work on any backend stuff without loading the frontend. You will just need to depend on tests instead of playing with it in the UI

@emlove
Copy link
Contributor

emlove commented Oct 18, 2017

Brainstorming idea:

What if we published the prebuilt polymer files on PyPI as an independent package? It could then be registered in frontend component using the existing REQUIREMENTS system. If the frontend component isn't configured with development: 1 it uses the resources from the PyPI package.

It's obviously messy that we'd have a PyPI package that only contains the polymer code, but we already include the frontend files in our current package. This would just be further separation of the two repos.

@rytilahti
Copy link
Member

Tbh, I would actually prefer to have some way to get the system running without first fighting quite a bit to get the front-end stuff to compile & working. However, it feels wrong to ship some auxiliary binary package to pypi. Therefore I would encourage that we need find a simple way to get UI bits compiled in a simple manner (wrt. pip install x) instead of going for dependency hacks on some pypi snapshot from the frontend.

In my opinion providing an option to use docker for this is not enough. Just my 0.02e, I applaud the idea of this PR completely so do not take this as a disagreement :-)

@emlove
Copy link
Contributor

emlove commented Oct 19, 2017

I'm mainly concerned with raising the barrier to entry for contribution, and personally would prefer some channel to ship the precompiled frontend to developers. If we ballpark by issue/PR count, over 95% of our contributions are to the backend. If people aren't actively developing the frontend, I don't see why we need everyone to rebuild it from source themselves. Especially since we are attempting to isolate the frontend from the backend, it makes sense that we'd move toward shipping it as it's own product.

Docker certainly will make building the frontend easier, but it's a big additional hurdle to people who are often starting their first foray into open source, and are not familiar with all of the tooling. I just want to make sure we carefully consider the ramifications of the final solution on casual developers.

@balloob
Copy link
Member Author

balloob commented Oct 19, 2017

I like the idea of publishing it as a pypi package a lot!

@balloob
Copy link
Member Author

balloob commented Oct 19, 2017

I've added initial work to convert it to a Python package: home-assistant/frontend@ef7911c

router.add_route(
'get', '/{}'.format(self.frontend_url_path), index_view.get)
router.add_route(
'get', '/{}/{{extra:.+}}'.format(self.frontend_url_path), index_view.get)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (85 > 79 characters)

if url not in _REGISTERED_COMPONENTS:
hass.http.register_static_path(url, path)
_REGISTERED_COMPONENTS.add(url)
@callback

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too many blank lines (2)

@balloob
Copy link
Member Author

balloob commented Oct 22, 2017

@armills last commit has changed it to importing the frontend from a PyPi package. It will not use the PyPi package when you pass in a path to the polymer repo:

frontend:
  development_repo: ../home-assistant-polymer

Will still need to fix the tests.

os.path.join(FINAL_PATH, "robots.txt"))
hass.http.register_static_path("/static", FINAL_PATH)
if hass.http.development:
hass.http.register_static_path("/home-assistant-polymer", POLYMER_PATH)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are now registering both dev & prod paths. Is this on purpose?

@@ -11,13 +11,6 @@ echo "Bootstrapping frontend..."
git submodule update --init
cd homeassistant/components/frontend/www_static/home-assistant-polymer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now homeassistant/components/frontend/home-assistant-polymer right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we're getting rid of the submodule completely, so devs will have this checked out at an arbitrary location they configure. I think we'd want to just remove script/bootstrap_frontend, and the docs will explain how to point to the other repo for development.

script/release Outdated
python3 setup.py sdist bdist_wheel upload
script/build_frontend

# python3 setup.py sdist bdist_wheel upload
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be uncommented?

@@ -294,26 +300,6 @@ def reload_themes(_):
descriptions[SERVICE_RELOAD_THEMES])


class BootstrapView(HomeAssistantView):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove it?

DATA_THEMES = 'frontend_themes'
DATA_DEFAULT_THEME = 'frontend_default_theme'
DEFAULT_THEME = 'default'

PRIMARY_COLOR = 'primary-color'

# To keep track we don't register a component twice (gives a warning)
_REGISTERED_COMPONENTS = set()
# _REGISTERED_COMPONENTS = set()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, and the comment above is no longer needed, right?

@andrey-git
Copy link
Contributor

Looks great!

Now, if I make a FE change and I would like to compile it and check that compiled code works - how do I do that?

@balloob
Copy link
Member Author

balloob commented Oct 22, 2017

Good question 🤔

So one way I have been doing it now is to run python3 setup.py sdist in polymer dir to generate a package. Then with HA virtualenv active, pip3 install ../home-assistant-polymer/dist/home-assistant-frontend-20171021.2.tar.gz --upgrade. Then start HASS with hass --skip-pip

I do realize that that's suboptimal. Guess we need another config option to load the compiled version from the repo. Shouldn't be too hard but for the sake of getting this monster merged, can we do that in another PR?

@andrey-git
Copy link
Contributor

Yeah this shouldn't block merging this PR

Maybe put a sticky message (not only regarding compiling FE, but about the whole new dev process) in #dev_frontend and #dev chat until this is fixed and new documentation is pushed to the site.

@emlove
Copy link
Contributor

emlove commented Oct 22, 2017

Also, brace yourselves for recommits of the polymer submodule in every PR.

'url_path': 'router'
}

assert self.hass.data[frontend.DATA_PANELS].get('weather') == {
assert self.hass.data[frontend.DATA_PANELS].get('weather').as_dict() == {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (81 > 79 characters)

@@ -53,20 +53,20 @@ def test_correct_config(self):
},
})

assert self.hass.data[frontend.DATA_PANELS].get('router') == {
assert self.hass.data[frontend.DATA_PANELS].get('router').as_dict() == {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (80 > 79 characters)

from homeassistant import setup
from homeassistant.components import panel_custom

from tests.common import get_test_home_assistant
from tests.common import get_test_home_assistant, mock_coro, mock_component

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'tests.common.get_test_home_assistant' imported but unused

hass.data[DATA_PANELS].pop(self.frontend_url_path)


self.webcomponent_url = \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too many blank lines (2)

'url_path': 'router'
}

assert self.hass.data[frontend.DATA_PANELS].get('weather') == {
assert self.hass.data[frontend.DATA_PANELS].get('weather').as_dict() == {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (81 > 79 characters)

@@ -53,20 +53,20 @@ def test_correct_config(self):
},
})

assert self.hass.data[frontend.DATA_PANELS].get('router') == {
assert self.hass.data[frontend.DATA_PANELS].get('router').as_dict() == {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (80 > 79 characters)

from homeassistant import setup
from homeassistant.components import panel_custom

from tests.common import get_test_home_assistant
from tests.common import get_test_home_assistant, mock_coro, mock_component

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'tests.common.get_test_home_assistant' imported but unused

hass.data[DATA_PANELS].pop(self.frontend_url_path)


self.webcomponent_url = \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too many blank lines (2)

@balloob
Copy link
Member Author

balloob commented Oct 22, 2017

Fixed the tests, this is ready for review.

register_built_in_panel(hass, panel)
# Finalize registration of panels that registered before frontend was setup
# This includes the built-in panels from line above.
yield from asyncio.wait([finalize_panel(panel) for panel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update hass.data[DATA_FINALIZE_PANEL] before this in case any panels register while this is waiting.

@@ -11,13 +11,6 @@ echo "Bootstrapping frontend..."
git submodule update --init
cd homeassistant/components/frontend/www_static/home-assistant-polymer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we're getting rid of the submodule completely, so devs will have this checked out at an arbitrary location they configure. I think we'd want to just remove script/bootstrap_frontend, and the docs will explain how to point to the other repo for development.

@balloob
Copy link
Member Author

balloob commented Oct 24, 2017

@armills @andrey-git shall we merge this?

Copy link
Contributor

@emlove emlove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! This is a huge step towards decoupling the frontend!

@balloob balloob merged commit 2bdad53 into dev Oct 25, 2017
@balloob balloob deleted the consolidate-frontend branch October 25, 2017 02:36
@andrey-git
Copy link
Contributor

http:
  development: 1

is still used for cache control.
Expected?

@andrey-git
Copy link
Contributor

When I want to run with FE dev mode I set development_repo in configuration.yaml pointing to home-assistant-polymer checked out from git.
I also run script/bootstrap there. Do I have to run script/build_frontend too?

It seems so, since mdi.html and serviceworker are not in the repo - they are only built by build_frontend.

@balloob
Copy link
Member Author

balloob commented Oct 25, 2017

Yes, build the frontend once. Removing Dev option from http component is on todo

@balloob balloob mentioned this pull request Nov 2, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants