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

[DRAFT/WIP] Publish stable types #20234

Closed
wants to merge 5 commits into from
Closed

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Oct 26, 2022

This isn't quite ready for prime time yet, but it does prove out the basic viability of @chadhietala's suggestion that we may not need to fix all the circularities before we can publish. We in fact can just publish as is: none of the circularities are hard blockers, at least for this part of the effort (though they will be for getting docs published with these as the source of truth).

The things we will need to do to be able to execute the rest of the way on this, given our goals:

  • Fix a couple cases where we are using private names in public types, specifically around the OWNER from @glimmer/owner.
  • Put the hacky generate-tsconfigs.mjs script somewhere besides the root, expand its capabilities to include wrapping generated modules in a declare module statement and running Prettier on the result, and rename it accordingly.
  • Land @wagenet's in-progress PR (Convert packages/ember to TS #20175) to convert the ember package to TS so we can publish types for it, which will unblock some of the other Ember packages as well.

An open question: do we want to try to properly exclude the private and internal parts of Ember's APIs? Publishing them may actually be preferable and even to some degree necessary for (officially-maintained) "collaborator" libraries which use private APIs.

This isn't remotely ready for primetime, but it does prove out the
basic viability of @chadhietala's suggestion that we may not need to
fix all the circularities before we can publish. We in fact can just
publish as is: none of the circularities are *hard* blockers, at least
for *this* part of the effort (though they will be for getting docs
published with these as the source of truth).

The things we will need to do to be able to execute the rest of the way
on this, given our goals:

- Fix a couple cases where we are using private names in public types,
  specifically around the `OWNER` from `@glimmer/owner`.

- Put the hacky `generate-tsconfigs.mjs` script somewhere besides the
  root, expand its capabilities to include wrapping generated modules
  in a `declare module` statement and running Prettier on the result,
  and rename it accordingly.

- Land @wagenet's in-progress PR (#20175) to convert the `ember`
  package to TS so we can publish types for it, which will unblock some
  of the other Ember packages as well.

- Properly exclude the parts of Ember's APIs we *don't* want to be
  publishing (all the purely-internal and private stuff).
This lets each config specify *only* how it actually does a build (or
not!), while sharing the config explicitly.
There are pre-existing runtime shenanigans in `@ember/object/internals`
to add debug-only errors to the class in dev builds. Those runtime
shenanigans produce the need for type-level shenanigans to match: TS
gets stuck here because the runtime shenanigans declare
`FrameworkObject` with a class expression (rather than the usual class
declaration form). That in turn means TS needs to be able to fully name
the type produced by the clsas expression, which includes the `OWNER`
symbol from `@glimmer/owner`.

By explicitly giving the declaration a type when assigning it the class
expression, instead of relying on inference, TS no longer needs to name
the `OWNER` property key from the super class, eliminating the private
name shenanigans.

Co-authored-by: Dan Freeman <dfreeman@salsify.com>
@chriskrycho
Copy link
Contributor Author

The remaining emit error is blocked on #20175.

There are at least two other issues flagged up by CI here:

  • A build error:

    You specified a `"srcDir": @ember/-internals/browser-environment` which does not exist and did not specify `"allowEmpty": true`.
    

    I don't believe that's related to this PR (it's unclear how it could be!), but will investigate further early next week.

  • A types error for @types/node on TS 4.9, fixed via [BUGFIX beta] update @types/node for TS 4.9 issue #20235.

- Add `.gitattributes` so GH will syntax highlight it correctly.
- Improve the naming, so: `tsconfig/compiler-options.json` etc.
@chriskrycho
Copy link
Contributor Author

Superseded by the actual implementation at #20275, which used the ideas from this but basically none of its implementation!

@chriskrycho chriskrycho deleted the spike-publishing-types branch November 22, 2022 02:35
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.

1 participant