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

fix: misc updates #132

Merged
merged 6 commits into from
Jan 20, 2022
Merged

fix: misc updates #132

merged 6 commits into from
Jan 20, 2022

Conversation

jimmywarting
Copy link
Contributor

@jimmywarting jimmywarting commented Jan 18, 2022

The purpose of this PR is:

I original wanted to improve the .d.ts to better return the types from the async from utilities. but ended up doing more.

This is what has changed:

  • Switched to using node-domexception - cuz formdata-polyfill used it anyway.
  • I improved the http loader with a basic cache mekanism to run the test cases faster.
  • Then i went ahead and switched all test cases to testharness so i could remove AVA
    • while doing so i tried to remove things WPT already cover
    • Also found a missing blob-constructor-any in WPT that we didn't use, so i included that for more test coverage.
      • that lead to a bug: that undefined wasn't casted to a string when it should be doing it... so i cast it to string first when textdecoder didn't do it.
  • I also applied some standard js code style to the repo
  • Also had to patch MessageChannel cuz wpt don't clean up the listener that prevents node from exiting.
  • lastly i bumped the patch version...
  • also included the fix from pr: fix(from, tests): Fix behavior of BlobDataItem.slice() #129

  • I Added unit tests (and removed some)

@jimmywarting jimmywarting marked this pull request as draft January 18, 2022 17:45
@codecov

This comment has been minimized.

@jimmywarting jimmywarting marked this pull request as ready for review January 18, 2022 18:36
@jimmywarting
Copy link
Contributor Author

jimmywarting commented Jan 18, 2022

fyi, i tested node's own blob against the WPT, it had some things that was failing...
will lookin to how we can conditionally extend builtin NodeJS blob at some point in the feature... it might be troublesome b/c it should work with URL.createObjectURL and files backed up by the file system also.

@jimmywarting jimmywarting requested a review from LinusU January 18, 2022 19:01
node: engines
- os: macOS-latest
node: "14"
node: ["17.3"]
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we include the currently supported versions of Node.js here?

Suggested change
node: ["17.3"]
node: ["12.20.0", "14.13.0", "16.0.0", "17"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The http module loader API changed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can revisit this later.

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Just one comment, otherwise LGTM 👍

@jimmywarting jimmywarting merged commit dc29588 into main Jan 20, 2022
@jimmywarting jimmywarting deleted the misc-updates branch January 20, 2022 13:00
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.

Behavior of .slice() is incorrect when using blobs backed by a file on the disk
2 participants