Skip to content

Add typescript usage test, fix typescript errors #111

Merged
matheus1lva merged 4 commits intoshellscape:masterfrom
rosskevin:ts-test
Feb 14, 2019
Merged

Add typescript usage test, fix typescript errors #111
matheus1lva merged 4 commits intoshellscape:masterfrom
rosskevin:ts-test

Conversation

@rosskevin
Copy link
Contributor

@rosskevin rosskevin commented Feb 12, 2019

Closes #110

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Please Describe Your Changes

  • the index.d.ts is invalid, and causes downstream ts users to be unable to build entirely, the errors cannot be ignored because the declarations are in-repo
  • remove declare module - this is not needed in-repo - only for out of repo declarations/augmentations
  • add ts usage test
  • add tooling to test usage
  • add needed types dependencies
  • hook into test script to prevent future errors

None of the above changes change any current tests or runtime code.

If it isn't too much trouble, please release this ASAP since the current master breaks any TS build currently.

@shellscape
Copy link
Owner

The tests for this are a welcomed addition and wonderful compliment to the project. I thank you!

@rosskevin
Copy link
Contributor Author

Added prettierrc and re-saved the files. I think it's good to go.

@rosskevin
Copy link
Contributor Author

This fixes a build-breaking change. I'm not trying to be pushy but our build is broken. Is there any chance to get this published today? If not I'll revert locally so our build can proceed.

@shellscape
Copy link
Owner

I just had a child born Monday morning and I'm short on time, so extra some patience will be needed. We like to get 2 or more approvals before merging. I'll be able to review the latest commit later this evening.

Copy link
Collaborator

@bebraw bebraw left a comment

Choose a reason for hiding this comment

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

Run with it. I would do 0.7.0 since it's a bigger change, though. 👍

semi: true
singleQuote: true
trailingComma: all
bracketSpacing: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why to override so many defaults?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. It's about matching previous style more or less.

@matheus1lva matheus1lva merged commit 118ab2a into shellscape:master Feb 14, 2019
@shellscape
Copy link
Owner

Finally got the chance to examine this. TSLint errors aren't being reported outside of the editor, meaning that any errors in the file aren't being caught by the npm script, and as such would always pass in the verification build step. Also removed the .prettierrc file - our prettier config is defined in eslint-config-shellscape. TSLint has it's own quote rule that will catch the quote style.

I'm working on cleaning that up now, but the build steo has to be fixed before we release.

@rosskevin rosskevin deleted the ts-test branch February 14, 2019 18:42
@rosskevin
Copy link
Contributor Author

@shellscape eslint is not prettier (and vice versa). I have prettier integrated as a vscode plugin, and on every single OSS project I maintain/contribute to. When I save a file, the vscode plugin will obey the prettier config in the project. When the config is not there, it will use the default prettier config, which will get you exactly the file that you nitpicked.

If you don't include a prettier config, it will continue to give similar problems to others. It is certainly your prerogative, but it mystifies me when e.g. one prefers fixing a trailing comma manually or with eslint fix vs automatically fixing it with prettier. I can say there is my life before prettier, and after prettier, and it is just not even a thought anymore as I start or contribute to other projects.

This will help with the eslint/prettier integration https://github.com/prettier/eslint-config-prettier if you are interested.

@shellscape
Copy link
Owner

The language is a bit strong, but I'll indulge. We simply have different preferences. I had a rather long response written but it basically boils down to that one fact. While we're happy to include a type definition file, this is not a TypeScript project. And the tools in the repo are setup for the sort of project it actually is. To that end, DefinitelyTyped might be a better home for it.

I would never block a PR for a dangling comma, but switching all quotes from one form to another is a relatively large change that typically stands out. I'd wager that anyone looking at the diff would see as much as well. I don't agree that not including a prettier config will cause the same kinds of issues in the future because you were kind enough to setup TSLint, which is dynamic enough to catch most of the formatting issues that would raise a flag moving forward.

If you're interested in our prettier ruleset that we use in conjunction with ESLint, you can find them here: https://github.com/shellscape/eslint-config-shellscape/blob/b0bbe3bc077664a13141e8010e27ca31a2d6c9e0/.eslintrc.json#L11-L14

I thank you again for contributing a major improvement to the project.

@rosskevin
Copy link
Contributor Author

My thought was to get a prettier config consistent with current styling, not proposing a style change...just proposing tooling that makes it easier for collaborators to fit-in.

@shellscape
Copy link
Owner

Given that TSLint alerts on error (in editor for those with editor plugins, when running npm run lint, and during CI) and that most editor plugins which support TSLint have a fix-on-save option, I think we've accomplished that without adding an additional prettier configuration file.

@shellscape shellscape mentioned this pull request Mar 19, 2019
18 tasks
smashercosmo pushed a commit to smashercosmo/webpack-plugin-serve that referenced this pull request Jul 23, 2019
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.

Typescript definitions fail downstream when used

4 participants