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

Public / Protected APIs #5599

Closed
paul-marechal opened this issue Jun 27, 2019 · 11 comments
Closed

Public / Protected APIs #5599

paul-marechal opened this issue Jun 27, 2019 · 11 comments
Labels
question user / developer questions

Comments

@paul-marechal
Copy link
Member

paul-marechal commented Jun 27, 2019

Some things are exposed in index.ts files under browser / electron / node etc... But some others aren't (WindowService?).

I was thinking about the "Stable APIs" item on the tentative 1.0 roadmap: maybe it would be easier to consider as "public" anything exposed under these index files, and anything that a user accesses directly would be considered internal and may break without much notice?

It is simple and it makes import blocks cleaner.

@paul-marechal
Copy link
Member Author

paul-marechal commented Jun 27, 2019

cc @svenefftinge @thegecko @benoitf any opinions?

@thegecko
Copy link
Member

That's a neat approach. May be hard to implement/manage/enforce automatically.

@paul-marechal
Copy link
Member Author

Maybe it's for the best: it will make us think about what to expose, since we'll have to be careful about it. Having stuff private is a good default IMO, since it avoids us committing to not change it.

@akosyakov
Copy link
Member

APIs are not something which is marked like public/protected but abstractions which are commonly used and have well-defined stable interface. The work of analysing possible changes and deciding whether something is common and stable enough is not possible to automate.

Exposing such abstractions in the code via index file or not it is another question. Right now i'm regretting of having index files at all since they tend to pull a lot of dependencies at runtime. For instance, if you want to run a test but import theia core in it then you load all reexported modules from core. It makes startup slow and bloat the memory. I would rather get rid of index files and use only absolute imports.

@akosyakov akosyakov added the question user / developer questions label Jul 1, 2019
@akosyakov
Copy link
Member

akosyakov commented Jul 1, 2019

Having stuff private is a good default IMO, since it avoids us committing to not change it.

First in JS there is not real private everything is accessible via obj['prop']. Second we want to provide customizability by default.

As a framework developers we commit to provide customizability and at the same time do our best to keep backward compatible when it makes sense. I understand that it makes contribution hard since changes should be reviewed thoroughly, but then we don't really want to shake existing code often. Such rules also should not be applied to everything ideally some of extensions will go away to own repos and we will work on reducing and stabilizing core.

@paul-marechal
Copy link
Member Author

First in JS there is not real private everything is accessible via obj['prop']. Second we want to provide customizability by default.

I was speaking conceptually, to not advertise all injectable components as "you should expect everything to be backward compatible", just to give us a bit of wiggle room.

[...] ideally some of extensions will go away to own repos and we will work on reducing and stabilizing core.

Sounds good, and with us slowly going toward 1.0, should we start discussing the few things we would break/refactor?

For instance I am not too happy with the way I implemented the VS Code task/shell execution compatibility directly into the terminal extension, and I was looking at moving code around, in the end it would make it more simple to use in other components. But it will change some internal APIs...

APIs are not something which is marked like public/protected but abstractions which are commonly used and have well-defined stable interface.

But then does that mean that any component added to the code base should be immutable in regards to its interface? Most services can work that way, but I know that in the terminal module some confusion happened during its design, and some things didn't really fit anymore. I would suspect that some other places are like that as well. Hence why I wonder if we should see at changing what needs to be changed for 1.0?

Exposing such abstractions in the code via index file or not it is another question. Right now i'm regretting of having index files at all since they tend to pull a lot of dependencies at runtime. For instance, if you want to run a test but import theia core in it then you load all reexported modules from core. It makes startup slow and bloat the memory. I would rather get rid of index files and use only absolute imports.

Makes sense, but then just rename/remove a file and you break API. Also, all other packages expose everything from their root, or some specific namespace. I really don't like the idea that people should know where a class is stored in the package file tree...

@akosyakov
Copy link
Member

akosyakov commented Jul 2, 2019

Sounds good, and with us slowly going toward 1.0, should we start discussing the few things we would break/refactor?

I thought for 1.0 we agreed to use VS Code extensions/Theia plugin APIs, but consider everything else as a subject to change? Right now is more important to finish VS Code extensions support. After 1.0 we can stabilize and document some abstractions from Theia core.

For instance I am not too happy with the way I implemented the VS Code task/shell execution compatibility directly into the terminal extension, and I was looking at moving code around, in the end it would make it more simple to use in other components. But it will change some internal APIs...

We are fine to break if changes are justified. Will such refactoring beneficial to support VS Code extensions? If not right now it will cause more disturbance than helping moving the project in the right direction. Please file an issue we can come back to it later after 1.0. We should get focused and have our priorities straight.

I really don't like the idea that people should know where a class is stored in the package file tree...

They should use proper tools to auto insert imports.

@akosyakov
Copy link
Member

We could follow Node.js approach, from here:

All functionality in the official Node.js documentation is part of the public API.

We will search for abstractions, stabilize and document them. During a review there will be an item that contributors and reviewers should ensure that documented (pubilc) APIs are never broken. Other APIs still can be broken without version 2.0. Later we can automate it with ts tooling via decorators.

cc @svenefftinge @marcdumais-work

@paul-marechal
Copy link
Member Author

Later we can automate it with ts tooling via decorators.

What would the decorators do/ensure?

@akosyakov
Copy link
Member

What would the decorators do/ensure?

I meant to mark concepts as APIs with annotations and have a ts based tooling analyzing such code for breaking changes. Something like what we have with Eclipse PDE api tooling.

@akosyakov
Copy link
Member

In Theia everything is API regardless to its visibility. We will use js-doc annotations like @stable, @experimental and @deprecated. It will be done as part of #7071

There would docs provided with explaining the meaning of each annotation, how it is related to Theia versioning, should be used and reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question user / developer questions
Projects
None yet
Development

No branches or pull requests

3 participants