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

Add create-react-app and puppeteer user tests #23471

Merged
merged 6 commits into from
Apr 17, 2018
Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Apr 17, 2018

This PR adds create-react-app and puppeteer to the user tests. I'm tracking the amount of work that would be needed to add a tsconfig to these projects with checkJs turned on.

create-react-app

create-react-app is a facebook package, so it requires yarn and a couple of subprojects are written in flow.

Errors

  1. Missing types for test harness (jest probably?).
  2. We don't let you require a json file.
  3. Need to check that url.parse returns an object with hostname defined before using hostname.
  4. Can't compare a string to a number. The string comes from a semver, so it's probably always a number.
  5. Promise.reject requires one parameter, a reason.
  6. fs.readFileSync returns a Buffer, not a string, with no encoding specified. Node's d.ts has a JSON.parse that doesn't accept Buffers, even though it usually works.
  7. Need to check that process.env["NODE_ENV"] is defined before using it index into an object.
  8. Node's d.ts doesn't have fs.F_OK, just fs.constants.F_OK. The former isn't documented.
  9. In checkRequiredFiles, we mark currentFilePath as use-before-defined because control flow doesn't understand that it will only ever be used if the body of the for loop executes and then throws after an initial assignment.
  10. We choose the wrong overload of execSync because the encoding: 'utf8' field of an object literal that's assigned to a variable widens to encoding: string.
  11. We don't recognise window as the global type, which contains encodeURIComponent. Should be fixed when we introduce a global type.
  12. Webpack hot reloading client relies on HotModuleReplacementPlugin to augment the module object, and we don't understand this pattern.

Test changes

  1. Skip flow packages.
  2. Override node module chalk that has a bad d.ts and is unmaintained.
  3. Add --ignore-scripts instead of making test harness use yarn. This means some babel dependencies aren't included, which bloats the baselines a little. But I don't want to make the harness any smarter.

Fixes

  1. Should allow importing json files, at least in javascript, at least as any. (TODO: Find PR)
  2. "Can't use types in JS" error should not block semantic errors in batch compilation. (In JS, type annotations should not block errors from the rest of the program #23472)
  3. Control flow isn't smart enough to understand that only fs.accessSync can throw, and that currentFilePath is always assigned in this case.
  4. Add a global type (Bind toplevel this assignments as global declarations #22891) We are waiting on TC39 to update the stage 3 proposal for the global keyword.
  5. Understanding side-effecting changes to module is pretty difficult and probably not worth it now.
  6. Our literal widening rules have been worked and re-worked, so I don't know if it's possible to get any better.

Result

Once things are up and running, the outcome is solid. Typescript catches 4 lint-y errors (3, 5, 6, 8) and 2 bugs-waiting-to-happen (4, 7). The other 6 are covered above. 50% is an acceptable ratio, I think.

Still, getting create-react-app to work would require some expertise to avoid chalk errors and to realise that flow errors were blocking semantic errors. After that, one error stands out as hard to fix: literal widening, which requires the author to know the closure type cast syntax (/** @type {'utf8'} */('utf8')).

I posted a stack overflow question to help people learn how to override bad types that ship with a package.

puppeteer

puppeteer is a google package that already uses typescript. I think it was ported from closure, because lots of the types use the !-prefix, which does nothing in typescript.

Errors

Types in the namespace Protocol are not found. I look around and could not find these either. That's it.

Fixes

In general, resolving types in closure-originating code is difficult. We could do better. Since I couldn't find the namespace Protocol myself, I can't propose any new ones this time.

Result

puppeteer already uses checkJs, so all the work has already been done.

@sandersn sandersn merged commit db68075 into master Apr 17, 2018
@sandersn sandersn deleted the add-app-user-tests branch April 17, 2018 18:21
@JoelEinbinder
Copy link
Contributor

For Puppeteer, protocol.d.ts is a generated file. You can create it by running node utils/protocol-types-generator in the Puppeteer folder. See puppeteer/puppeteer#2325

@sandersn
Copy link
Member Author

Thanks @JoelEinbinder. I got a clean compile with 2.8.1 this way. I think the bogus errors are worth the simplicity of the test harness though. I don't want to add arbitrary pre- and post- steps at this point.

@microsoft microsoft locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants