/h

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

Remove the need to specify the Chrome extension ID when building with bundled assets #2901

Merged
merged 9 commits into from Feb 3, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@robertknight @nickstenning
@robertknight
Contributor

robertknight commented Feb 1, 2016

This simplifies building the Chrome extension and should make things simpler when we have extensions for other browsers in future by removing the need to specify the 'chrome-extension://' URL when bundling assets.

This URL was previously used to reference scripts and styles in app.html and embed.js. In app.html this is no longer needed following the decoupling of the service URL from the <base> URL, so assets can just use root-relative paths (eg. /public/scripts/...)

For embed.js, the extension add a <meta> tag to the document containing the root URL of the Hypothesis app and embed.js constructs asset URLs relative to this. The reason for using a <meta> tag is that this is accessible by scripts running on the page but in isolated worlds.

robertknight added some commits Jan 22, 2016

@robertknight
Enable building Chrome extension without specifying ID 
This simplifies the command to build the extension and
also paves the way for easier building of multiple browser
extensions in future by removing the need to specify
the absolute URL (`chrome-extension://<ID>/`) at which
bundled assets are found.

 * Render app.html with root-relative paths instead of
   absolute paths. This is now possible since the
   decoupling of the service URL for the Hypothesis
   service from the <base> URL for the page.

 * Render embed.js.jinja2 with root-relative URLs
   in the Chrome extension build and pass in
   the 'chrome-extension://<ID>/' prefix in from
   the extension itself.

 * Remove the '--assets' argument from buildext.py and
   replace it with a '--no-bundle-sidebar' flag which
   uses assets from $SERVICE_URL/ instead.
d28cde5
@robertknight
Simplify the instructions for building the Chrome extension 
The Chrome extension can now be built specifying just the URL of the
Hypothesis service and the result will work on sites that use CSP
or are served via HTTPS.

The docs continue to provide instructions on how to
build an extension that loads assets served by the web app,
but make that an advanced path for developers seeking a faster testing
workflow and keep the steps for newcomers as simple as possible.
6b664bd
@robertknight
Add a test to verify injection of the <meta> tag 
Add a test to verify that the 'hypothesis-resource-root'
<meta> tag is correctly injected into the DOM
d1276fc
@robertknight
Change the default service URL to https://hypothes.is/ 
This further removes the need to specify a '--service' URL
in order to build a working Hypothesis Chrome extension.

Running 'hypothesis-buildext chrome' will produce a
working extension that uses the production H service.
2fe77cb
@robertknight
Update documentation in buildext.py 
The default mode of operation is now to bundle the assets
into the Chrome extension.
be6a35d
@robertknight
Fix Chrome extension build with '--debug' flag 
When building the extension with the '--debug' flag enabled,
which skips the uglifyjs step defined in assets.yaml,
this resulted in an invalid URL being generated for
app.js in app.html.
30730ea
@robertknight
Log a useful hint if the WebSocket connection is rejected 
The WebSocket only accepts connections from a whitelist of
origins. When connecting to a local H service, provide
a useful hint when connections (eg. from a local Chrome
extension build with bundled assets) are rejected due
to the H service not having the sidebar app's origin
whitelisted.
Loading status checks…
7afec83
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Feb 3, 2016

Contributor

This looks great in principle, but I get embed.js:17 Uncaught TypeError: Failed to construct 'URL': Invalid base URL on /docs/help now.
Contributor

nickstenning commented Feb 3, 2016

This looks great in principle, but I get embed.js:17 Uncaught TypeError: Failed to construct 'URL': Invalid base URL on /docs/help now.
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Feb 3, 2016

Contributor

There's also a slight problem (?) where if you build the extension without --debug and with --no-bundle-sidebar then it will request .min.js URLs from the server, which aren't built in development.

Although I'm happy to accept that in this case "build the extension with --debug" is a valid solution.
Contributor

nickstenning commented Feb 3, 2016

There's also a slight problem (?) where if you build the extension without --debug and with --no-bundle-sidebar then it will request .min.js URLs from the server, which aren't built in development.

Although I'm happy to accept that in this case "build the extension with --debug" is a valid solution.
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Feb 3, 2016

Contributor

Last problem. If I build the extension with:

hypothesis-buildext --debug --service https://localhost:5000 --no-bundle-sidebar chrome

and then force the assets build with:

hypothesis assets conf/development.ini

then I still get a 404 from https://localhost:5000/assets/webassets-external/ad0808dd0f264894e56caee80ae26d21_icomoon.css when loading the extension.
Contributor

nickstenning commented Feb 3, 2016

Last problem. If I build the extension with:

hypothesis-buildext --debug --service https://localhost:5000 --no-bundle-sidebar chrome

and then force the assets build with:

hypothesis assets conf/development.ini

then I still get a 404 from https://localhost:5000/assets/webassets-external/ad0808dd0f264894e56caee80ae26d21_icomoon.css when loading the extension.

robertknight added some commits Feb 3, 2016

@robertknight
Avoid resolving resource URLs relative to a base URL if none is speci… 
…fied

When Hypothesis is loaded on pages via the bookmarklet or embed,
rather than via the Chrome extension, no 'hypothesis-resource-root'
tag will be present on the page so resourceRoot will be empty.

In this case, resolve() should just return the input URL rather
than trying to resolve it.
Loading status checks…
a796253
@robertknight
Use correct asset URLs in embed.js when building without bundled sidebar 
When rendering embed.js, the bundled asset URLs should be referenced
regardless of whether app.html is being loaded from the service
or the Chrome extension.
Loading status checks…
d5e9a18
@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Feb 3, 2016

Contributor

This looks great in principle, but I get embed.js:17 Uncaught TypeError:
Failed to construct 'URL': Invalid base URL on /docs/help now

Fixed

There's also a slight problem (?) where if you build the extension without --debug
...
Last problem. If I build the extension with:
hypothesis-buildext --debug --service https://localhost:5000 --no-bundle-sidebar chrome
and then force the assets build with:
hypothesis assets conf/development.ini

Fixed. The problem in both cases was that the URLs generated by webassets in the webassets environment created in buildext.py to render embed.js didn't match those used in the web service.

For now I've changed the build so that embed.js will always reference bundled versions of hypothesis.(css|js) and dependencies regardless of whether app.html is loaded from the Chrome extension or from the service.
Contributor

robertknight commented Feb 3, 2016

This looks great in principle, but I get embed.js:17 Uncaught TypeError:
Failed to construct 'URL': Invalid base URL on /docs/help now

Fixed

There's also a slight problem (?) where if you build the extension without --debug
...
Last problem. If I build the extension with:
hypothesis-buildext --debug --service https://localhost:5000 --no-bundle-sidebar chrome
and then force the assets build with:
hypothesis assets conf/development.ini

Fixed. The problem in both cases was that the URLs generated by webassets in the webassets environment created in buildext.py to render embed.js didn't match those used in the web service.

For now I've changed the build so that embed.js will always reference bundled versions of hypothesis.(css|js) and dependencies regardless of whether app.html is loaded from the Chrome extension or from the service.
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Feb 3, 2016

Contributor

Super! LGTM.
Contributor

nickstenning commented Feb 3, 2016

Super! LGTM.

nickstenning added a commit that referenced this pull request Feb 3, 2016

@nickstenning
Merge pull request #2901 from hypothesis/remove_assets_ext_build_arg 
Remove the need to specify the Chrome extension ID when building with bundled assets
aa41ada

@nickstenning nickstenning merged commit aa41ada into master Feb 3, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@nickstenning nickstenning deleted the remove_assets_ext_build_arg branch Feb 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment