-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Typed plugins in new platform #12857
Conversation
598f3e7
to
4783b64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just few nits/questions.
packages/kbn-types/README.md
Outdated
|
||
Now external code can use this package, e.g. something like this: | ||
|
||
```js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional nit: maybe typescript
instead of js
? Webstorm supports it (and also complains if we try to highlight typescript with js
hint), but not sure if it's standard and your IDE supports it as well, so up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
packages/kbn-types/.gitignore
Outdated
@@ -0,0 +1 @@ | |||
types/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what is the difference between packages/@elastic/...package-name...
and packages/...package-name...
? Should we put kbn-types
package under @elastic
or is there any reason why we shouldn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it to @elastic
. We'll likely never push anything that's not under @elastic
, so don't really see the point of that folder, but it's something we can solve later
}); | ||
|
||
const router = http.createAndRegisterRouter('/api/foo', {}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ;
question:: also wondering if we should include TS in packages/...
while running npm run prettier
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 done
packages/kbn-types/README.md
Outdated
|
||
If you want to play around with an example, see the `./example` folder. | ||
|
||
## Exposing types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this doc 👍
I think this is the simplest way to build external plugins with types. Added a
kbn-types
package that contains a file that exports whatever we want to export from core, then use TS to build declaration files. See the readme in thekbn-types
package for more info.I'll create issues for some follow-ups. One is having CI build the package and make sure the error code is 0, as we otherwise have broken external types. Not really sure how to otherwise test this, e.g. making sure the right things are available etc. Another follow-up is exploring if we should commit the types, e.g. to make "breaking changes" to the types more obvious.
I've got a feeling it should be possible to further minimise the exposed types, but if this works, I think this is probably a good enough start, then we can learn as we start trying to make core plugins truly "independent" (separate package.json etc).