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

Add Cache SW manual test infrastructure #6532

Merged
merged 8 commits into from
Dec 12, 2016

Conversation

jridgewell
Copy link
Contributor

Fixes #6495.

/cc @dvoytenko for the extensions script url changes

@cramforce
Copy link
Member

Looks good including the move of the extension code into a file with less deps. But don't you need to update some unit tests there?

@jridgewell
Copy link
Contributor Author

But don't you need to update some unit tests there?

Yup, working on that next.

@dvoytenko
Copy link
Contributor

@jridgewell Are there actually any changes to how we calculate extension URL? Or is it just refactoring to a separate module?

@jridgewell
Copy link
Contributor Author

No changes to how they're done, just extracted and added calculateEntryPointScriptUrl.

For some god awful reason, all of our polyfills were being pulled into
the SW code because of the dependency chains.
The old code prevented a base directory from being used, like if we
were serving from `http://localhost/dist`
@jridgewell jridgewell force-pushed the cahce-sw-manual-testing branch 2 times, most recently from bfc33f9 to 528873f Compare December 9, 2016 20:20
@jridgewell
Copy link
Contributor Author

PTAL

Copy link
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

LGTM with one comment.

@@ -108,7 +108,7 @@ runner.run('Cache SW', () => {
'https://cdn.ampproject.org/rtv/123/v0.js');
});

it('rewrites v1 to versioned v1', () => {
it.skip('rewrites v1 to versioned v1', () => {
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we don't match v1 right now, only v0.

@jridgewell jridgewell merged commit 658f410 into ampproject:master Dec 12, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* Extract extensions-location from extensions-impl

For some god awful reason, all of our polyfills were being pulled into
the SW code because of the dependency chains.

* Create calculateEntryPointScriptUrl

* Ensure SW installs max code when asked

* Update pathname rewriting in SW

The old code prevented a base directory from being used, like if we
were serving from `http://localhost/dist`

* Add Cache SW manual test infrastructure

* Remove unused export

* Add tests

* Fix tests
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* Extract extensions-location from extensions-impl

For some god awful reason, all of our polyfills were being pulled into
the SW code because of the dependency chains.

* Create calculateEntryPointScriptUrl

* Ensure SW installs max code when asked

* Update pathname rewriting in SW

The old code prevented a base directory from being used, like if we
were serving from `http://localhost/dist`

* Add Cache SW manual test infrastructure

* Remove unused export

* Add tests

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants