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: Improve type safety for browser-compatible modules #995

Merged
merged 7 commits into from
Jul 6, 2021
Merged

fix: Improve type safety for browser-compatible modules #995

merged 7 commits into from
Jul 6, 2021

Conversation

jsejcksn
Copy link
Contributor

@jsejcksn jsejcksn commented Jul 1, 2021

This PR fixes the current compiler errors emitted when using browser-compatible TypeScript modules without including the Deno namespace library in compilerOptions.lib.


There is a link to contributing guidelines toward the end of the Deno repo README, which includes this:

Document and maintain browser compatibility.

If a module is browser compatible, include the following in the JSDoc at the top of the module:

// This module is browser compatible.

Maintain browser compatibility for such a module by either not using the global Deno namespace or feature-testing for it. Make sure any new dependencies are also browser compatible.

This statement is not explicit about type compatibility, but I think it is natural for the type safety of browser-compatible modules to be aligned in the same way.

I searched for all files including the mentioned text using this method:

# cd to repo directory
grep -Rl "This module is browser compatible." .

For each file, I checked the diagnostic output of the compiler for warnings/errors using a browser-compatible TSConfig with Deno. (The TSConfig is presently included in the PR, but can be removed if desired.) If diagnostic messages were emitted for a file, I addressed them in the source. Most of the changes involve modifications to conditional expressions used to determine whether or not a Deno method or property exists.

@kt3k
Copy link
Member

kt3k commented Jul 1, 2021

For each file, I checked the diagnostic output of the compiler for warnings/errors using a browser-compatible TSConfig with Deno.

I think we need to add a new step in .github/workflows/ci.yaml to check these.

something like:

deno cache --config browser-compat.tsconfig.json _util/os.ts
deno cache --config browser-compat.tsconfig.json fmt/colors.ts
...

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Jul 1, 2021

I think we need to add a new step in .github/workflows/ci.yaml to check these.

@kt3k This makes sense. Have any ideas on how to programmatically generate the list of matching modules and insert into the workflow file so that manual maintenance is not needed?

@kt3k
Copy link
Member

kt3k commented Jul 1, 2021

@jsejcksn I guess something like the below might work

grep "This module is browser compatible" . -Rl | xargs deno cache --config browser-compat.tsconfig.json

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Jul 1, 2021

@kt3k I tried adding a step for type checking, but got an error like this:

error: Expected ';', '}' or <eof> at file:///home/runner/work/deno_std/deno_std/.github/workflows/ci.yml:11:11

At first, I thought it was because the YAML file was matching, too, but even after applying a variation to ignore the ./.github directory, the error persisted.

Feel free to make a code change suggestion or add a commit to this PR if you think you can make it work. This was my last addition before reverting:

- name: Type check browser compatible modules
  run: grep -Rl --exclude-dir=./.github "// This module is browser compatible." . | xargs deno cache --config browser-compat.tsconfig.json

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.

@jsejcksn Thank you for your contribution! LGTM

@kt3k kt3k merged commit 91cd23a into denoland:main Jul 6, 2021
@jsejcksn
Copy link
Contributor Author

jsejcksn commented Jul 6, 2021

@kt3k Thanks for fixing the workflow file

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