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

TS types output #1304

Merged
merged 5 commits into from
Nov 15, 2018
Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Nov 12, 2018

Summary

supports #1262

  • removes src from published code
  • collates all types from .t.ds,.ts, and .tsx from src into a top-level eui.d.ts file for publishing
  • introduces a top-level types directory which is git- and npm-ignored, this allows the typescript binary to have a place to emit definitions & transpiled files, as turning emit off creates other problems.

Tested by linking EUI into kibana and modifying the new eui.d.ts file. I also removed the src directory in the linked build to ensure it was no longer used.

This relies on dts-generator for the dirty work of combining all definitions into the top-level eui.d.ts. The implementation on this branch (scripts/dtsgenerator.js) is overkill but is the correct setup to allow authoring pure typescript files. The changes in this PR are pulled over from my in-progress typescript branch (https://github.com/chandlerprall/eui/tree/feature/typescript) which uses this setup to support both existing .d.ts and .tsx files.

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs
- [ ] Documentation examples were added

  • A changelog entry exists and is marked appropriately
    - [ ] This was checked for breaking changes and labeled appropriately
    - [ ] Jest tests were updated or added to match the most common scenarios
    - [ ] This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@pugnascotia
Copy link
Contributor

Sorry, clicked close by mistake.

Anyway, the code looks reasonable. I ran the build and it created eui.d.ts as expected. I couldn't see the types directory, does that get cleaned up somehow?

I noticed that there are still triple-slash directives in eui.d.ts - might that be a problem? It's confusing if nothing else, because the file paths in the directives are wrong.

@chandlerprall
Copy link
Contributor Author

The types dir isn't cleaned up - by removing noEmit": true most times the tsc compiler is executed it writes to types. My IDE's typescript integration is one thing that triggers it. Ideally it shouldn't really be a thing; I haven't yet found a way to guarantee avoiding all the ways it gets created.

I'll double check that the triple-slash references aren't a problem, or if they can be excluded from the generated file.

@chandlerprall
Copy link
Contributor Author

@pugnascotia the /// <reference lines weren't effecting anything, but I've updated the dtsgenerator script to remove them after building to keep it clean.

Copy link
Contributor

@pugnascotia pugnascotia left a comment

Choose a reason for hiding this comment

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

Run the result through prettier?

/ducks

Joking, LGTM.

@jasonrhodes
Copy link
Member

jasonrhodes commented Nov 14, 2018

LGTM too, I notice that the jsdoc style comments are imported in and then show up in VS Code.

screen shot 2018-11-14 at 2 16 50 pm

The path for the @see annotation is possibly a bit confusing but I'm not sure there's a) much to do there easily and b) if it would even be helpful to update the path. As long as that's expected, I say 🚢 🇮🇹

@chandlerprall chandlerprall merged commit 1c837b7 into elastic:master Nov 15, 2018
@chandlerprall chandlerprall deleted the dedicated-types-output branch November 15, 2018 19:00
@snide snide mentioned this pull request Nov 30, 2018
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants