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

Remove StaticCharAtlas support (fixes #1576) #1580

Closed
wants to merge 1 commit into from

Conversation

bgw
Copy link
Member

@bgw bgw commented Jul 24, 2018

This defaults to the dynamic atlas and leaves in NoneCharAtlas and the experimentalCharAtlas option (with 'static' removed).

This leaves me with a few questions:

  1. Do we want to completely remove support for NoneCharAtlas and experimentalCharAtlas? NoneCharAtlas provides a nice example of the modularity of the atlas system, even though it doesn't make much sense to use.

  2. If we want remove the experimentalCharAtlas option, should we leave the typedef there (ignoring any values passed) for backwards compatibility? I think semver requires this, though it's not a runtime concern.

  3. If we want to keep experimentalCharAtlas, should we rename it to charAtlas? What should we do if someone passes in an experimentalCharAtlas option? Should we leave 'static' in there as an alias to 'dynamic' for compatibility? I think semver requires this.

  4. Unfortunately I don't think we have any established standard for warning about deprecated features. Is it appropriate to use console.warn in a library context like this? My guess is no.

This defaults to the dynamic atlas and leaves in NoneCharAtlas and the
experimentalCharAtlas option (with 'static' removed).

This leaves me with a few questions:

1. Do we want to completely remove support for NoneCharAtlas and
   experimentalCharAtlas? NoneCharAtlas provides a nice example of the
   modularity of the atlas system, even though it doesn't make much
   sense to use.

2. If we want remove the experimentalCharAtlas option, should we leave
   the typedef there (ignoring any values passed) for backwards
   compatibility? I think semver requires this, though it's not a
   runtime concern.

3. If we want to keep experimentalCharAtlas, should we rename it to
   charAtlas? What should we do if someone passes in an
   experimentalCharAtlas option? Should we leave 'static' in there as an
   alias to 'dynamic' for compatibility? I think semver requires this.

4. Unfortunately I don't think we have any established standard for
   warning about deprecated features. Is it appropriate to use
   `console.warn` in a library context like this? My guess is no.
@Tyriar Tyriar added this to the 3.7.0 milestone Jul 24, 2018
@Tyriar
Copy link
Member

Tyriar commented Jul 24, 2018

@bgw

  1. I guess we can keep NoneCharAtlas as it's so small, plus it will be useful to debug whether issues are caused by the dynamic atlas.
  2. We could add @deprecated to the comment and a console.warn when it's set for 3.6, anything labelled experimental doesn't need to follow semver. I think mainly just Hyper and VS Code are using dynamic which are easily fixed during the update.
  3. Yeah let's rename to charAtlas.
  4. I normally add @deprecated to the function in .d.ts, a console.warn can't hurt too.


declare const Promise: any;

export interface IOffscreenCanvas {
Copy link
Member

Choose a reason for hiding this comment

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

👍 to removing this, the shared/ idea has been replaced with #1507

@Tyriar
Copy link
Member

Tyriar commented Jul 24, 2018

anything labelled experimental doesn't need to follow semver

Adding to the readme about experimental API as I think that was either just in my head or in various release notes 😄 #1581

@Tyriar Tyriar added the work-in-progress Do not merge label Jul 30, 2018
@Tyriar Tyriar modified the milestones: 3.7.0, 3.8.0 Sep 4, 2018
@Tyriar Tyriar modified the milestones: 3.8.0, 3.9.0 Oct 1, 2018
@bgw bgw closed this Nov 24, 2018
@Tyriar Tyriar removed this from the 3.9.0 milestone Nov 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants