Skip to content

Conversation

@JakeChampion
Copy link
Contributor

@JakeChampion JakeChampion commented May 20, 2022

The below constructors will now throw an error if called as plain functions as per their specifications:

  • CompressionStream
  • Headers
  • Request
  • Response
  • TextDecoder
  • TextEncoder
  • TransformStream
  • URL
  • URLSearchParams

These constructors can now only be called via new or Reflect.construct

I wasn't sure what we want to do with the Fastly specific constructors such as CacheOverride so I have left them as-is

The below constructors will now throw an error if called without `new` as per their specifications:
- CompressionStream
- Headers
- Request
- Response
- TextDecoder
- TextEncoder
- TransformStream
- URL
- URLSearchParams
@JakeChampion JakeChampion changed the title Throw error if specific constructors are called without new Throw error if specific constructors are called as plain functions May 20, 2022
@JakeChampion JakeChampion marked this pull request as ready for review May 20, 2022 15:10
@JakeChampion
Copy link
Contributor Author

I thought this would increase the amount of passed tests from web-platform-tests but it looks like this part of the WebIDL specification wasn't being fully tested within web-platform-tests.

I've made a pull-request to web-platform-tests which tests this part, if/when that gets merged I can update our version of web-platform-tests and then see how many more tests we pass 👍

Copy link
Contributor

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

I have two notes that should be fairly simple to address before landing this:

  1. I think we should apply this to Fastly-specific constructors as well
  2. how would you feel about introducing a CTOR_HEADER macro, parallel to the METHOD_HEADER one, and using that instead?

The obvious difference is that we can't infer the name to use from __func__, but that seems ok to me.

…uctors have correct number of arguments and not called as regular functions
@JakeChampion
Copy link
Contributor Author

This is great, thank you!

I have two notes that should be fairly simple to address before landing this:

1. I think we should apply this to Fastly-specific constructors as well

2. how would you feel about introducing a `CTOR_HEADER` macro, parallel to the `METHOD_HEADER` one, and using that instead?

The obvious difference is that we can't infer the name to use from __func__, but that seems ok to me.

Oh that is a good suggestion! ☺️
I've made those changes and pushed it up now 👍

Copy link
Contributor

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

Thank you!

@tschneidereit
Copy link
Contributor

Oh, turns out URLSearchParams doesn't require any arguments, so we have a bunch of test failures :)

@JakeChampion
Copy link
Contributor Author

Oh, turns out URLSearchParams doesn't require any arguments, so we have a bunch of test failures :)

Hooray for tests! 👯

@tschneidereit
Copy link
Contributor

Hooray for tests! 👯

Indeed!

@tschneidereit tschneidereit merged commit 4153163 into fastly:main May 25, 2022
@JakeChampion JakeChampion deleted the patch-1 branch May 25, 2022 15:28
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.

2 participants