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

Add a proposed API check option #2921

Merged
merged 2 commits into from
May 11, 2020
Merged

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented May 8, 2020

Part of #2826

@Tyriar Tyriar added this to the 4.7.0 milestone May 8, 2020
@Tyriar Tyriar requested a review from jerch May 8, 2020 19:42
@Tyriar Tyriar self-assigned this May 8, 2020
Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

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

👍 LGTM.

I just wonder if collecting them under an .experimental property, that does this check once for all members, would be easier? And later on register the member on the main API as Terminal.XY, maybe with a transit mode for one or two version, where it is still reachable under Terminal.experimental.XY), so embedders have time to adopt.

While it would to separate things more clearly - not sure if we need this.

@Tyriar
Copy link
Member Author

Tyriar commented May 10, 2020

We could have both, there's an issue for that too. I really like a clear opt in this has where the apis just won't work until you flip the switch.

@Tyriar
Copy link
Member Author

Tyriar commented May 10, 2020

Also the still reachable under experimental thing puts extra burden on us, needing to keep 2 implementations for a bit. Less burden is what I'm going for 🙂

@jerch
Copy link
Member

jerch commented May 10, 2020

@Tyriar Yepp, the extra experimental stuff does not help much I guess, also if we'd go only with that, embedders have a hard time to fix the shifted symbols later on. So lets keep it simple.

@Tyriar Tyriar modified the milestones: 4.7.0, 4.6.0 May 11, 2020
@Tyriar Tyriar merged commit 7fb885c into xtermjs:master May 11, 2020
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.

2 participants