feat: add option to enable cross-origin isolation #234
Conversation
nolanlawson
force-pushed
the
cross-origin-isolation
branch
from
7101af7 to
e01f5d9
Compare
|
Fixed the merge conflicts! |
src/server.ts
Outdated
| @@ -91,6 +92,7 @@ export class Server { | |||
| this.server = server; | |||
| const app = new Koa(); | |||
|
|
|||
| app.use(crossOriginIsolation()); | |||
There was a problem hiding this comment.
Is there a chance this change could be breaking in the case where someone is explicitly measuring something where external resources are requested?
In this case, may it be worth putting this change behind a ServerOpts option, where the default is the current behavior (not cross origin isolated), but the headers can be opted in?
There was a problem hiding this comment.
Good point! I totally didn't think of that. 😅
I think making this disabled by default sort of defeats the purpose of improving the timing accuracy, but I also admit I don't have any evidence that CORP/COEP actually does this (maybe the browser treats localhost differently? not sure). That said, having the option at least unblocks SharedArrayBuffer and friends.
So sure thing: I added a CLI option, --cross-origin-isolated, which is disabled by default. I think further experimentation would be needed to see if it has any effect on timing accuracy. 🙂
There was a problem hiding this comment.
I think the benefits of having it (even if behind a flag):
- Safe roll-out of the option. Making it a non-breaking change.
- The timers should be more accurate when cross origin isolated is enabled.
- And we could still add the
SharedArrayBuffer(and friends) features. They would just have to check thecrossOriginIsolatedproperty.
I'm a fan of this change!
nolanlawson
changed the title
|
Missed exposing the CLI option, but fixed it ^. Tested manually and it appears to be working – the headers are sent only when the |
| @@ -872,3 +872,4 @@ tach http://example.com | |||
| | `--trace` | `false` | Enable performance tracing ([details](#performance-traces)) | | |||
| | `--trace-log-dir` | `${cwd}/logs` | The directory to put tracing log files. Defaults to `${cwd}/logs`. | | |||
| | `--trace-cat` | [default categories](./src/defaults.ts) | The tracing categories to record. Should be a string of comma-separated category names | | |||
| | `--cross-origin-isolated` | `false` | Add HTTP headers to enable [cross-origin isolation](https://developer.mozilla.org/en-US/docs/Web/API/Window/crossOriginIsolated). | | |||
There was a problem hiding this comment.
Nice --cross-origin-isolated option addition! One last thing that I can think of is to add this change to the CHANGELOG under unreleased. Here's another PR as an example: https://github.com/google/tachometer/pull/239/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR8-R12
src/test/server_test.ts
Outdated
There was a problem hiding this comment.
Thank you for adding tests!
|
Looks like the error above might be CI-related? |
Fixes #233.
I added a test to confirm that the headers work. I also verified manually that
window.crossOriginIsolatedwas true in Chrome.