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

Removing ?pipe=sub opt-in pattern #15680

Open
jugglinmike opened this issue Mar 5, 2019 · 7 comments
Open

Removing ?pipe=sub opt-in pattern #15680

jugglinmike opened this issue Mar 5, 2019 · 7 comments

Comments

@jugglinmike
Copy link
Contributor

The wptserve project provides text substitution functionality under two conditions:

  • the request specifies the "pipe" query string parameter which includes the string "sub"
  • the string .sub. appears somewhere in the name of the requested resource

I'd like to remove the former for two reasons.

First, it's difficult to verify statically. Errors of omission have been made, but such mistakes could be avoided with a simple name/content-based heuristic if only the .sub. method were available.

Second, it adds complexity (which we need to test, document, and expect contributors to understand) without providing value. Comparing the two opt-in mechanisms, the query string is generally more flexible than file name because it gives authors a choice about when/how to enable a feature. However, that flexibility is inappropriate for this feature because files aren't written to work both with and without substitution.

There may be other use cases that I'm not aware of, though. Do any of the infrastructure maintainers know something about that?

@zcorpan
Copy link
Member

zcorpan commented Mar 12, 2019

One consideration is that substitution could be combined with other pipes (e.g. trickle), and we should verify that it works to combine them when mixing the file name convention for sub and query string for other pipes. (I recall that this didn't work a few years ago, but I don't know if it does now.)

@zcorpan
Copy link
Member

zcorpan commented Mar 12, 2019

cc @jgraham

@foolip
Copy link
Member

foolip commented Mar 15, 2019

#9345 is an issue where the combination didn't work as expected. In light of that and #15805 I'd be happy to remove this.

@youennf
Copy link
Contributor

youennf commented Apr 26, 2019

See also #16569 where ?pipe=sub was sometimes not used when including referrer-policy-test-case.js

jugglinmike added a commit to bocoup/wpt that referenced this issue Aug 17, 2019
The "sub" functionality of wptserve's "pipe" feature has previously been
identified as a candidate for removal [1]. While it can generally be
replaced with a filename-based pattern, the optional argument can only
be specified with the query-string-based approach.

The file `static-import.js` in the import-maps test suite is the only
existing usage of the optional argument. Refactor it to use a Python
handler so that a future patch may replace the remaining "pipe"-based
usages with filename-based usages.

[1] web-platform-tests#15680
@jugglinmike
Copy link
Contributor Author

@zcorpan: thank you for calling that out. The .sub. filename pattern does not work with the trickle pipe command. That might not matter, though. Can you and @jgraham tell me what you think?

The sub and trickle "pipe" commands work together:

$ echo '{{host}}' > host.txt; curl 'http://web-platform.test:8000/host.txt?pipe=sub|trickle(d0)'
web-platform.test

...but he .sub. pattern does not work with the trickle command:

$ echo '{{host}}' > host.sub.txt; curl 'http://web-platform.test:8000/host.sub.txt?pipe=trickle(d0)'
{{host}}

Then again, the sub and trickle command are a bit tricky to use at the same time. The sub command consumes the entire response stream, so if you swap the order (as in ?pipe=trickle(d0)|sub), you can no longer observe most of the "trickling."

Judging from the current usage in WPT (and if you trust a regexp for this sort of thing), no one needs to use both features simultaneously:

$ git grep -E 'sub.*trickle|trickle.*sub'
loading/lazyload/iframe-loading-eager.tentative.html:  <iframe id="below_viewport" src="resources/subframe.html?pipe=trickle(d1)" loading="eager" width="200px" height="100px" onload="below_viewport_iframe_onload();">
$

So do you think we need support, e.g. host.sub.txt?pipe=trickle(d0) before we eliminate pipe=sub?

@zcorpan
Copy link
Member

zcorpan commented Aug 20, 2019

Since I recalled it being a problem, I thought I had written at least one such test. But it's possible that it has since been changed so that it no longer needs both.

I can't argue with the grep, so I think until someone does need both, it makes sense to just document what sub does and does not work with. And file issues for pipe combinations that we think should work but currently don't (like #9345 ).

@jugglinmike
Copy link
Contributor Author

This initially seemed like a case of "you ain't gonna need it," but the more I thought about it, the more I felt like it was just a rough edge waiting to surprise someone. Unless I'm mistaken, the fix is pretty straightforward, so I've submitted gh-18623 to address this.

jugglinmike added a commit to bocoup/wpt that referenced this issue Nov 1, 2019
The "sub" functionality of wptserve's "pipe" feature has previously been
identified as a candidate for removal [1]. While it can generally be
replaced with a filename-based pattern, the optional argument can only
be specified with the query-string-based approach.

The file `static-import.js` in the import-maps test suite is the only
existing usage of the optional argument. Refactor it to use a Python
handler so that a future patch may replace the remaining "pipe"-based
usages with filename-based usages.

[1] web-platform-tests#15680
jugglinmike added a commit that referenced this issue Nov 4, 2019
The "sub" functionality of wptserve's "pipe" feature has previously been
identified as a candidate for removal [1]. While it can generally be
replaced with a filename-based pattern, the optional argument can only
be specified with the query-string-based approach.

The file `static-import.js` in the import-maps test suite is the only
existing usage of the optional argument. Refactor it to use a Python
handler so that a future patch may replace the remaining "pipe"-based
usages with filename-based usages.

[1] #15680
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 29, 2019
…rce, a=testonly

Automatic update from web-platform-tests
[import-maps] Re-implement dynamic resource (#18648)

The "sub" functionality of wptserve's "pipe" feature has previously been
identified as a candidate for removal [1]. While it can generally be
replaced with a filename-based pattern, the optional argument can only
be specified with the query-string-based approach.

The file `static-import.js` in the import-maps test suite is the only
existing usage of the optional argument. Refactor it to use a Python
handler so that a future patch may replace the remaining "pipe"-based
usages with filename-based usages.

[1] web-platform-tests/wpt#15680
--

wpt-commits: 287f2dd5e5d1fb42156d37ddf539385cfe54269e
wpt-pr: 18648
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Nov 29, 2019
…rce, a=testonly

Automatic update from web-platform-tests
[import-maps] Re-implement dynamic resource (#18648)

The "sub" functionality of wptserve's "pipe" feature has previously been
identified as a candidate for removal [1]. While it can generally be
replaced with a filename-based pattern, the optional argument can only
be specified with the query-string-based approach.

The file `static-import.js` in the import-maps test suite is the only
existing usage of the optional argument. Refactor it to use a Python
handler so that a future patch may replace the remaining "pipe"-based
usages with filename-based usages.

[1] web-platform-tests/wpt#15680
--

wpt-commits: 287f2dd5e5d1fb42156d37ddf539385cfe54269e
wpt-pr: 18648
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Nov 30, 2019
…rce, a=testonly

Automatic update from web-platform-tests
[import-maps] Re-implement dynamic resource (#18648)

The "sub" functionality of wptserve's "pipe" feature has previously been
identified as a candidate for removal [1]. While it can generally be
replaced with a filename-based pattern, the optional argument can only
be specified with the query-string-based approach.

The file `static-import.js` in the import-maps test suite is the only
existing usage of the optional argument. Refactor it to use a Python
handler so that a future patch may replace the remaining "pipe"-based
usages with filename-based usages.

[1] web-platform-tests/wpt#15680
--

wpt-commits: 287f2dd5e5d1fb42156d37ddf539385cfe54269e
wpt-pr: 18648

UltraBlame original commit: 61cc669a41be928b94ab1a36e6b6120caa07bffc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Nov 30, 2019
…rce, a=testonly

Automatic update from web-platform-tests
[import-maps] Re-implement dynamic resource (#18648)

The "sub" functionality of wptserve's "pipe" feature has previously been
identified as a candidate for removal [1]. While it can generally be
replaced with a filename-based pattern, the optional argument can only
be specified with the query-string-based approach.

The file `static-import.js` in the import-maps test suite is the only
existing usage of the optional argument. Refactor it to use a Python
handler so that a future patch may replace the remaining "pipe"-based
usages with filename-based usages.

[1] web-platform-tests/wpt#15680
--

wpt-commits: 287f2dd5e5d1fb42156d37ddf539385cfe54269e
wpt-pr: 18648

UltraBlame original commit: 61cc669a41be928b94ab1a36e6b6120caa07bffc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Nov 30, 2019
…rce, a=testonly

Automatic update from web-platform-tests
[import-maps] Re-implement dynamic resource (#18648)

The "sub" functionality of wptserve's "pipe" feature has previously been
identified as a candidate for removal [1]. While it can generally be
replaced with a filename-based pattern, the optional argument can only
be specified with the query-string-based approach.

The file `static-import.js` in the import-maps test suite is the only
existing usage of the optional argument. Refactor it to use a Python
handler so that a future patch may replace the remaining "pipe"-based
usages with filename-based usages.

[1] web-platform-tests/wpt#15680
--

wpt-commits: 287f2dd5e5d1fb42156d37ddf539385cfe54269e
wpt-pr: 18648

UltraBlame original commit: 61cc669a41be928b94ab1a36e6b6120caa07bffc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants