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

No dynamic import in service worker #27699

Merged
merged 10 commits into from Feb 24, 2021
Merged

No dynamic import in service worker #27699

merged 10 commits into from Feb 24, 2021

Conversation

@jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Feb 19, 2021

@jgraham this PR adds a new "service worker module" handler type, but I'm not confident I've done it correctly. The intent is to allow .any.serviceworker-module.html to run the equivalent .any.js in a service worker with type 'module'.

Part of whatwg/html#6395 and w3c/ServiceWorker#1356

@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Feb 19, 2021

@domenic you might be interested in this, as it's a pattern that could be extended to other module worker types

@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Feb 19, 2021

@annevk feedback addressed, cheers!

Copy link
Member

@annevk annevk left a comment

Looks good modulo that seemingly fundamental problem. I don't feel comfortable reviewing the infrastructure addition. I'll leave that to @jgraham and @stephenmcgruer.

@domenic
Copy link
Member

@domenic domenic commented Feb 19, 2021

The runs at e.g. https://github.com/web-platform-tests/wpt/pull/27699/checks?check_run_id=1936236036 show the new infrastructure isn't working right on CI---maybe that's as expected though.

The runs at https://github.com/web-platform-tests/wpt/pull/27699/checks?check_run_id=1936236067 and https://github.com/web-platform-tests/wpt/pull/27699/checks?check_run_id=1936051438 show that Firefox and Safari do support dynamic import, it seems.

@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Feb 22, 2021

@domenic those results look expected to me. No browsers support module service workers right now. And yes, dynamic import works (but not as intended) in Firefox and Safari.

@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Feb 22, 2021

I've gone ahead and added support for dedicated/shared worker modules too. May as well do the lot.

Copy link
Contributor

@jgraham jgraham left a comment

Basically I think this implementation is reasonable; just one comment.

if key == "title":
value = value.replace("\\", "\\\\").replace('"', '\\"')
return 'self.META_TITLE = "%s";' % value
return None


class AnyWorkerModuleHandler(AnyWorkerHandler):

This comment has been minimized.

@jgraham

jgraham Feb 22, 2021
Contributor

I feel like I'd prefer if this and AnyWorkerHandler both inherited from a common base rather than having this one override parts of the behaviour in the other concrete class.

@wpt-pr-bot wpt-pr-bot requested review from caitp, jdm and zqzhang Feb 23, 2021
@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Feb 23, 2021

@jgraham heh, I forgot that Python auto-exports. I've renamed the other references.

@jakearchibald jakearchibald requested a review from jgraham Feb 23, 2021
@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Feb 23, 2021

@jgraham the failures seem unrelated to me (webrtc?), but I'm not really familiar with them

@jgraham
Copy link
Contributor

@jgraham jgraham commented Feb 24, 2021

OK, I'm pretty sure I've seen that test failure in unrelated PRs, so it's probably broken on master. Will investigate.

@jgraham
Copy link
Contributor

@jgraham jgraham commented Feb 24, 2021

Ah @stephenmcgruer already fixed it in #27744

@jgraham jgraham closed this Feb 24, 2021
@jgraham jgraham reopened this Feb 24, 2021
@jakearchibald jakearchibald merged commit a02460e into master Feb 24, 2021
46 checks passed
46 checks passed
update-pr-preview
Details
update-pr-preview
Details
update-pr-preview
Details
build-and-publish
Details
build-and-publish
Details
build-and-tag
Details
build-and-tag
Details
Azure Pipelines Build #20210224.30 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (infrastructure/ tests: macOS) infrastructure/ tests: macOS succeeded
Details
Azure Pipelines (tools/ unittests: Windows + Python 3.6) tools/ unittests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/ unittests: Windows + Python 3.8) tools/ unittests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/ unittests: macOS + Python 3.6) tools/ unittests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/ unittests: macOS + Python 3.8) tools/ unittests: macOS + Python 3.8 succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows + Python 3.6) tools/wpt/ tests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows + Python 3.8) tools/wpt/ tests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS + Python 3.6) tools/wpt/ tests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS + Python 3.8) tools/wpt/ tests: macOS + Python 3.8 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows + Python 3.6) tools/wptrunner/ unittests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows + Python 3.8) tools/wptrunner/ unittests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS + Python 3.6) tools/wptrunner/ unittests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS + Python 3.8) tools/wptrunner/ unittests: macOS + Python 3.8 succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
download-firefox-nightly Community-TC (pull_request)
Details
infrastructure/ tests (Python 3) Community-TC (pull_request)
Details
lint Community-TC (pull_request)
Details
resources/ tests (Python 3.6) Community-TC (pull_request)
Details
resources/ tests (Python 3.8) Community-TC (pull_request)
Details
sink-task Community-TC (pull_request)
Details
tools/ integration tests (Python 3.6) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.8) Community-TC (pull_request)
Details
tools/ unittests (Python 3.6) Community-TC (pull_request)
Details
tools/ unittests (Python 3.8) Community-TC (pull_request)
Details
wpt-chrome-dev-results Community-TC (pull_request)
Details
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
wpt-chrome-dev-stability Community-TC (pull_request)
Details
wpt-decision-task Community-TC (pull_request)
Details
wpt-firefox-nightly-results Community-TC (pull_request)
Details
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@jakearchibald jakearchibald deleted the sw-no-dynamic-import branch Feb 24, 2021
@jakearchibald
Copy link
Contributor Author

@jakearchibald jakearchibald commented Feb 24, 2021

@jgraham thanks!

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