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

refactor: mark modules as browser compatible #1972

Merged
merged 14 commits into from
Mar 1, 2022
Merged

refactor: mark modules as browser compatible #1972

merged 14 commits into from
Mar 1, 2022

Conversation

jsejcksn
Copy link
Contributor

@jsejcksn jsejcksn commented Mar 1, 2022

These modules are useful in browser environments.

@jsejcksn jsejcksn changed the title refactor(streams/delimiter): mark module and deps as browser compatible refactor: mark modules as browser compatible Mar 1, 2022
@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

Are there even more that can be included in this PR?

@crowlKats
Copy link
Member

@jsejcksn
streams/merge.ts
async/deferred.ts

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

streams/merge.ts

@crowlKats It appears that the asyncIterator prevents compat for that module:

% deno cache --config ./browser-compat.tsconfig.json streams/merge.ts
Check file:///deno_std/streams/merge.ts
error: TS2504 [ERROR]: Type 'ReadableStream<T>' must have a '[Symbol.asyncIterator]()' method that returns an async iterator.
            for await (const data of stream) {
                                     ~~~~~~
    at file:///deno_std/streams/merge.ts:22:38

@crowlKats
Copy link
Member

Then TS' typings are outdated. async iter on readablestream is part of spec

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

Then TS' typings are outdated. async iter on readablestream is part of spec

@crowlKats I understand and agree, but the tests still fail and prevent inclusion until TS updates its DOM types.

@crowlKats
Copy link
Member

crowlKats commented Mar 1, 2022

Oh well, then we can skip on that one for now

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

The same TS typings problem exists for uuid v4:

% deno cache --config ./browser-compat.tsconfig.json uuid/mod.ts
Check file:///deno_std/uuid/mod.ts
error: TS2339 [ERROR]: Property 'randomUUID' does not exist on type 'Crypto'.
  return crypto.randomUUID();
                ~~~~~~~~~~
    at file:///deno_std/uuid/v4.ts:30:17

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

This PR turned out to be a bit bigger than I initially anticipated. Please review and let me know if I overlooked anything: thanks!

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

Oh, we have many compatible modules! Nice work! @jsejcksn

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

@crowlKats Is there a "todo" list in this repo's files/issues/etc. where the outdated DOM types issues mentioned above can be noted for revisiting after a future update to the TS DOM lib?

@kt3k
Copy link
Member

kt3k commented Mar 1, 2022

@jsejcksn I don't think we have such issue

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

@kt3k Can one be created? (Perhaps using the projects or wiki features if an issue is not appropriate — I realize many orgs don't leave issues open which are not yet actionable. However, it seems very useful to record this information in a way that increases the likelihood of its being seen again to be revisited.)

@kt3k
Copy link
Member

kt3k commented Mar 1, 2022

@jsejcksn I think issue is the best for tracking it. Would you mind creating one?

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

I think issue is the best for tracking it. Would you mind creating one?

@kt3k Done

@kt3k
Copy link
Member

kt3k commented Mar 1, 2022

@jsejcksn Thanks! Nice!

@kt3k kt3k merged commit 58204e6 into denoland:main Mar 1, 2022
@kt3k
Copy link
Member

kt3k commented Mar 1, 2022

Modified the above issue a little bit, and made the remaining items into todo list (in github md notation). Feel free to edit them if you find any new items

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Mar 1, 2022

@kt3k I updated it again to sort at the top level by incompatible types, with affected modules listed under each, including links to the latest relevant lines in the TS DOM lib files.

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.

3 participants