Skip to content

Add axios' source to user tests #23490

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

Merged
merged 2 commits into from
Apr 17, 2018
Merged

Add axios' source to user tests #23490

merged 2 commits into from
Apr 17, 2018

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Apr 17, 2018

This PR adds the source of axios 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. We already have axios' npm install in the user tests under axios/, since it's a popular package that ships a d.ts — we want to avoid breaking users of the package.

I skipped checking everything except lib: dist, examples, test and sandbox.

  1. Dist: Understanding bundled, minified code is not a goal here
  2. Examples: Only errors here were strict null checks, which isn't appropriate for example code.
  3. Tests: The tests don't have any interesting errors — it's just missing jasmine assertions and testing things that would be caught by a type system anyway.
  4. Sandbox: I'm not sure this code is even part of the build. It has a couple of errors that are duplicates of other errors.

Errors

  1. We don't let you import package.json.

  2. url.parse returns a UrlObjectCommon, whose fields are all optional. Doesn't seem like anybody remembers to check for undefined, though. The documentation doesn't mention it either.

  3. JSDoc optionality only adds undefined to the type, not null. So passing null isn't allowed.

  4. Assigning a value of a different type to a variable isn't allowed without declaring the variable as a union.

  5. We don't even allow you to check window.XDomainRequest for existence. Maybe window should have a string indexer. Would be fixed by Bind toplevel this assignments as global declarations #22891 if window: global & Window.

  6. We don't understand that util.extend adds properties to its argument via assignment.

  7. We don't recognise variables initialised with call expressions as namespaces (In JS, any declaration with a non-primitive initializer should be a JS container #22637).

  8. We don't understand aliasing of this and subsequent assignments as adding instance properties.

  9. We don't treat parameters as namespaces that can be special-assigned to. See (7).

  10. undefined << 8 is not allowed even though it's well-defined.

  11. Prototype assignment followed by prototype property assignment doesn't allow addition of prototype properties (In JS, function declarations should allow subsequent prototype assignment #23007).

  12. We expect jsdoc for arguments to be an array type, but the author of merge in utils.js thought of it as the type of "the first parameter" and gives a scalar type.

  13. (unrelated to this PR) axios.d.ts uses export default even though code in sandbox uses var axios = require('../index')

  • Strict null checks: (1, 3, 4, 5)
  • Lint-ish: (3, 4, 5, 11, 13)
  • Declarations/aliases: (6, 7, 8, 9, 10, 12)

Note that I just saw (1, 2) in create-react-app too.

Result

This result is not great. There aren't a lot of errors, but none of them are really useful, and 6 of 12 are a failure to understand complex aliasing of declarations. Most of those aliases would require a fundamental overhaul of Javascript binding since the aliasing assignments would have to be understood first, and then property-assignment-as-declaration understood second.

We already have the npm-installed version in order to test their d.ts so
that we don't break their users.
@sandersn sandersn merged commit e26745f into master Apr 17, 2018
@sandersn sandersn deleted the add-app-user-tests-2 branch April 17, 2018 22:20
@microsoft microsoft locked and limited conversation to collaborators Jul 30, 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.

2 participants