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

Enhance typing of postMessage in Service Workers #25176

Closed
3 of 4 tasks
nikeee opened this issue Jun 23, 2018 · 7 comments
Closed
3 of 4 tasks

Enhance typing of postMessage in Service Workers #25176

nikeee opened this issue Jun 23, 2018 · 7 comments
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@nikeee
Copy link
Contributor

nikeee commented Jun 23, 2018

Search Terms

  • postMessage
  • Transferable
  • structuredserialize, structured serialize
  • use-after-transfer
  • service worker

Suggestion

The Service Worker APIs come with a mthod postMessage (e.g. window.postMessage). Three things:

  1. The parameter message is processed using the structured clone algorithm, which requires the value to
    satisfy certain constrains: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

    • Current type of message: any
    • The algorithm prohibits passing these values for message (or an object containing these values):
      • Errors and Functions
      • DOM nodes
    • There is a list of all allowed types. However:
      • No symbols
      • only plain objects (e.g. from object literals)
    • While I think that it would be possible to constrain the message type from any to something more specific, I don't know if there is a way to constrain the object requirements.
    • any allows passing not-serializable data
    • Suggestion: Constrain the type of message
    • Additional suggestion: Constrain the type of MessageEvent#data
  2. There is a Transferable interface which is used to provide transfering objects instead of copying them.

    • It has no specific properties or methods
    • It is currently implemented by: ArrayBuffer, MessagePort, ImageBitmap
    • Tricky: As Transferable is basically an empty interface
    • any allows passing not-transferable data
    • Suggestion: Constrain the type of transfer/transferList to ArrayBuffer | MessagePort | ImageBitmap (or find another way of constraining the type)
  3. A transfered object cannot be used after it was transferred (e.g. using postMessage())

    • Suggestion: Introduce a diagnostic for use-after-transfer of a variable to warn the user

Use Cases

  • For 1: Prevents accidental passing of an object containing not-serializable data. Prevents an exception during runtime (DATA_CLONE_ERR).
  • For 2: Prevents passing a value of the wrong type that does not implement the interface.
  • For 3: Prevents accidental use of value after it was transfered.

Examples

1.:
These errors could be prevented (all valid according to the current definitions):

window.postMessage({
	f() {
		alert("hi!");
	}
}, "*");

window.postMessage({
	e: document.createElement("div"),
}, "*");

window.postMessage({
	s: Symbol(":("),
}, "*");

window.postMessage(class foo { }, "*");

2.:
This error could be prevented:

window.postMessage({ }, "*", [ { foo: 42 } /* objects are not transferable */]);
// -> Uncaught TypeError: Failed to execute 'postMessage' on 'Window':
// Value at index 0 does not have a transferable type.

3.:
This unintentional use-after-transfer could be prevented / pointed at:

const bar = new ArrayBuffer(42);

window.postMessage({a: 13}, "*", [bar]);

// Prints 0, because it was used after transfer (presumably not the intention of the user)
console.log(bar.byteLength); 

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript / JavaScript code
    • Well, it would break if it surfaces previously undetected type errors
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. new expression-level syntax)
@mhegazy
Copy link
Contributor

mhegazy commented Jun 25, 2018

We should be able to strongly type the postMessage function today using a union type of allowed types (with the exception of Object), e.g.:

type Transferable =
    | number
    | string
    | boolean
    | null
    | undefined
    | Date
    | ImageData
    | Number
    | Boolean
    | String
    | Date
    | RegExp
    | Blob
    | File
    | FileList
    | ArrayBuffer
    | ArrayBufferView
    | Map<unknown, unknown>
    | Set<unknown>;

declare function postMessage(message: Transferable | Transferable[], targetOrigin?: string, transfer?: any[]): void;

As for the side effect tracking, or use after transfer, i am afraid the compiler is not equipped to do this sort of analysis at the time being.

@mhegazy mhegazy added Suggestion An idea for TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this labels Jun 25, 2018
@mhegazy mhegazy added this to the Community milestone Jun 25, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Jun 25, 2018

PRs welcomed!

@nikeee
Copy link
Contributor Author

nikeee commented Jun 25, 2018

Cool! I'd file a PR for 1 and 2 as soon as I have some time.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 26, 2018

The change will need to be done in https://github.com/Microsoft/TSJS-lib-generator

@mhegazy mhegazy self-assigned this Jul 17, 2018
@mhegazy mhegazy added the Fixed in TSJS repo Fix merged in https://github.com/Microsoft/TSJS-lib-generator, but not ported yet label Jul 17, 2018
@nikeee
Copy link
Contributor Author

nikeee commented Jul 19, 2018

This issue is only partially fixed by the PR in the TSJS repository. I don't think the message parameter cannot be properly typed without recursive types. I think leaving it any would suffice for now.

@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed Fixed A PR has been merged for this issue labels Jul 25, 2018
@mhegazy mhegazy removed the Fixed in TSJS repo Fix merged in https://github.com/Microsoft/TSJS-lib-generator, but not ported yet label Jul 31, 2018
@felixfbecker
Copy link
Contributor

The PR that closed this did not fix this. The signature still takes any.

@nikeee
Copy link
Contributor Author

nikeee commented Feb 16, 2019

The PR only fixed the transfer parameter, not the message parameter (as said in the previous comments).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants