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

Convert the addon to TypeScript #23

Merged
merged 1 commit into from
May 19, 2020

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented May 14, 2020

  • Add ember-cli-typescript to build TS assets, and expect-type to verify the types exported publicly.

  • Convert implementation and tests to TypeScript. Add type tests to constrain the public API.

  • Add docstrings to the publicly exported items for better editor feedback.

  • Update the README:

    • Elaborate the TS docs, including full TS examples, explanations, and important caveats.
    • Add a table of contents.
    • Fix some grammatical issues.
    • Move the explanation of what the addon is above its note on the original contributors.

Supersedes and therefore resolves #9, and thereby also resolves #8.

@chriskrycho
Copy link
Contributor Author

As a follow-on for this (early next week) I will add support for testing against a range of TypeScript versions, including @next, so that we can provide strong stability guarantees for consumers of the addon.

I propose the following guidance around supported TypeScript versions:

  1. Initially support TypeScript versions 3.6, 3.7, and 3.9:

    • TypeScript 3.6 was released August 28, 2019—just 10 days after the release of Ember 3.12 LTS, so this policy would directly align the supported TypeScript versions of this project with currently supported Ember versions.

    • TypeScript 3.8 is excluded because it was a fairly unstable release, and simply broke against Ember's types; as such the Typed Ember Team recommended against adopting it to all consumers.

  2. Maintain support for TypeScript versions aligning roughly to Ember's LTS cycle. TypeScript releases occur quarterly, which means every other TypeScript release happens at approximately the same time as an Ember LTS. This does not commit the addon to supporting any particular TypeScript version: if the version has serious stability issues, especially related to Ember's types (as did 2.9, 3.1, and 3.8), we may decline to support it.

  3. Define the requirements for supporting new TypeScript versions as follows: support a new version—

    • does not require a release if there are no required changes; but the documented range of supported versions should be updated
    • requires a bugfix release if the types could be changed in a backwards-compatible way (that is: updating the type definitions to support the new version continues to pass the type tests for the supported range)
    • requires a breaking change if the types cannot be changed in a backwards-compatible way
  4. Given (2) and (3), commit not to making breaking changes for only TypeScript, to minimize churn and impact on the ecosystem. Instead, align TypeScript-related breaking changes to when an Ember LTS goes out of support.

cc. @pzuraq @dfreeman @jamescdavis @rwjblue

@rwjblue
Copy link
Member

rwjblue commented May 14, 2020

In general, that policy seems good to me overall though I don't know that the 4th point will work in practice (if users can't make a modifier using ember-cli-typescript and we can fix it, we probably should do the major bump release regardless of Ember's LTS cycle).

@chriskrycho
Copy link
Contributor Author

@rwjblue that's fair for sure; that was definitely the most conservative aspect of this. (To be clear the only thing being blocked would be upgrading to a new TS version, rather than continuing to use e-c-ts as normal, but even so!)

On reflection, though, maybe we aim to be that conservative only around dropping supported versions? It's not so big a deal to cut a breaking change that has a minor change; I take it to be a slightly (?) bigger deal to remove a supported version from the matrix.

addon/-private/class/modifier-manager.ts Show resolved Hide resolved
addon/-private/class/modifier.ts Show resolved Hide resolved
addon/-private/class/modifier-manager.ts Show resolved Hide resolved
addon/-private/class/modifier.ts Show resolved Hide resolved
addon/-private/functional/modifier.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
types/ember-private/index.d.ts Show resolved Hide resolved
types/global.d.ts Outdated Show resolved Hide resolved
@rwjblue
Copy link
Member

rwjblue commented May 14, 2020

To be clear, I think we should work on landing this without the refactors that I mention in my comments (though we should still make a todo issue and link to it in the code), but there are some more fundamental things that I do think we should fixup before landing (allowJs and ember-cli-babel version)

@rwjblue
Copy link
Member

rwjblue commented May 14, 2020

On reflection, though, maybe we aim to be that conservative only around dropping supported versions? It's not so big a deal to cut a breaking change that has a minor change; I take it to be a slightly (?) bigger deal to remove a supported version from the matrix.

Ya, this makes the most sense to me. Revving majors is "fine" (for the most part), but we do have to be somewhat careful about the poor merging behavior that ember-cli gives us by default (though that is an issue for a different repo 😝).

.gitignore Outdated Show resolved Hide resolved
- Add `ember-cli-typescript` to build TS assets, and `expect-type` to
  verify the types exported publicly.

- Convert implementation and tests to TypeScript. Add type tests to
  constrain the public API.

- Add docstrings to the publicly exported items for better editor
  feedback.

- Update the README:
  - Elaborate the TS docs, including full TS examples, explanations, and
    important caveats.
  - Add a table of contents.
  - Fix some grammatical issues.
  - Move the explanation of what the addon *is* above its note on the
    original contributors.
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

The changes here look good to me, thanks for working through all of my annoying quibbles @chriskrycho!

I am curious what the path forward is for the versioning issue. How do we properly test on supported TypeScript versions? Should we consider landing this with an intentionally invalid types entry in package.json so that we can land the refactor while we work through the actual policy issues?

@@ -0,0 +1 @@

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this empty file? What does it do for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't! I'll remove it!

@chriskrycho
Copy link
Contributor Author

@rwjblue my thought on landing this was that we would do basically this sequence:

  1. Land this PR.
  2. Land the PR I'm working on right now which supports testing types against the support matrix of stable versions.
  3. Define the policy issues—probably in parallel with (2).
  4. Publish.
  5. Refactor.

That said, I'm perfectly fine with switching the order and doing (5) before (3), and accordingly not publishing types.

@rwjblue
Copy link
Member

rwjblue commented May 19, 2020

I don’t have a strong preference other than that we have to deal with the SemVer / TS issue before publishing types officially. I believe that we are on the same page here though, so I’m perfectly happy to go in whatever order you prefer.

@rwjblue rwjblue merged commit 4dad9d2 into ember-modifier:master May 19, 2020
@chriskrycho
Copy link
Contributor Author

100% agreed that we have to nail that down before publishing! See #30 – I'll publish a draft there of the revised proposal I came up with after conversation here and talking with other Typed Ember folks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing types?
3 participants