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

[UPDATE] Updates Glimmer-VM and Typescript #220

Merged
merged 3 commits into from
Nov 9, 2019

Conversation

pzuraq
Copy link
Member

@pzuraq pzuraq commented Nov 6, 2019

Updates the Glimmer-VM, and Typescript/ember-cli-typescript. The update
to TS was necessary due to updates from the Glimmer-VM. Updating ec-typescript
to make sure that published types near the same version that it supports also
seems to make sense.

@mike-north
Copy link
Contributor

Following a discussion w/ @pzuraq , here are some of my suggestions to help (partially/marginally) insulate consumers of this library from type-related breakage (mostly due to the large number of breaking changes associated with the 2.9.x -> 3.5.x TS compiler bump)

  • Add some positive and negative test cases for the types, using either https://github.com/SamVerschueren/tsd or https://github.com/microsoft/dtslint
    • dtslint will require some special configuration, because by default it always runs tests against typescript@next which can be noticeably unstable at times
    • tsd does not have any built-in functionality for testing a range of compiler versions like dtslint does
  • Use api-extractor to detect unintentional exports and changes to the public API surface (i.e., accidental leakage of private types through the public API)
  • Create an ember-try-like script that runs the aforementioned test suite with each version of the TS compiler that this library aims to support (keeping in mind that developers who are not writing TS, but author code in a ts-aware editor are often impacted by breakage around type information)

@pzuraq
Copy link
Member Author

pzuraq commented Nov 6, 2019

To be clear, prior to this change, types were being compiled with TS 3.2.3, not 2.9.x. This is part of the reason I bumped the version of ec-typescript, the actual compiled types for the built packages are currently supplied by broccoli-typescript-compiler (same across the whole monorepo), and we're just using ec-typescript to strip types with Babel.

Overall though, I agree with the rest of the points, I think it's good idea to add type tests now since they're effectively going to be part of the public API for these packages. Will add them to this PR, before the upgrade commits, so we can ensure no breakage is happening.

@mike-north
Copy link
Contributor

mike-north commented Nov 6, 2019

Overall though, I agree with the rest of the points, I think it's good idea to add type tests now since they're effectively going to be part of the public API for these packages. Will add them to this PR, before the upgrade commits, so we can ensure no breakage is happening.

sounds like a good idea. I'd make a quick script to ensure the tests are run w/ TS 3.2.3 so that we know they remain compatible. This is a shell script I use for a travis-ci test matrix to validate against multiple TS versions

Copy link
Contributor

@mike-north mike-north left a comment

Choose a reason for hiding this comment

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

type testing LGTM

Chris Garrett added 2 commits November 8, 2019 12:02
Updates the Glimmer-VM, and Typescript/ember-cli-typescript. The update
to TS was necessary due to updates from the Glimmer-VM. Updating ec-typescript
to make sure that published types near the same version that it supports also
seems to make sense.
Copy link
Contributor

@tomdale tomdale left a comment

Choose a reason for hiding this comment

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

This looks great. I am happy to merge. However, would you mind including a few paragraphs in the README or somewhere else giving a high-level mental model for how to think about these type tests and the workflow associated with them.

For example, from my understanding, adding a new property to the Component class will cause the type test to fail. If I want to add new API, what is my workflow for doing that? How do I run the tests to verify my changes? How and when do we test for internal implementation details, e.g. we're currently testing the existence of two private symbols—how should I as a contributor think about things like that? Links to external resources would be fine as well. Just so we all have a shared mental model.

isDestroyed: unknown,
willDestroy: unknown,

// These are not public API, but technically part of the shape
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale for including type tests for these?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no way to not include them in the type check, unfortunately, it would throw an error since they exist on the object.

@tomdale tomdale merged commit dc9f630 into glimmerjs:master Nov 9, 2019
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