The Wayback Machine - https://web.archive.org/web/20201207135414/https://github.com/seek-oss/sku/pull/535
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

Update Typescript and ESLint #535

Merged
merged 3 commits into from Sep 22, 2020
Merged

Update Typescript and ESLint #535

merged 3 commits into from Sep 22, 2020

Conversation

@mattcompiles
Copy link
Contributor

@mattcompiles mattcompiles commented Sep 21, 2020

No description provided.

@mattcompiles mattcompiles requested a review from seek-oss/sku-maintainers as a code owner Sep 21, 2020
@changeset-bot
Copy link

@changeset-bot changeset-bot bot commented Sep 21, 2020

🦋 Changeset is good to go

Latest commit: 0b468f2

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mattcompiles
Copy link
Contributor Author

@mattcompiles mattcompiles commented Sep 21, 2020

Seems the @typescript-eslint/explicit-module-boundary-types rule is going to give a lot of trouble to react projects if they haven't been written that way from the beginning. It's only a warning as well which is mostly useless. Thoughts on disabling at the sku level? Or should we raise in eslint-config-seek?

@jahredhope
Copy link
Member

@jahredhope jahredhope commented Sep 21, 2020

I can see the benefit of explicitly declaring your return types.
Does the rule allow you to cast the function to a React.FunctionalComponent?

import React, { FunctionComponent } from 'react';

const App: FunctionalComponent<props> = () => {}

If so, and we believe it's a good pattern I'd suggest changing from warning to error in eslint-config-seek.
Alternatively I don't mind disabling the rule for sku/React projects as it's common pattern to assume a React component boundary is a React component.

Basically anything other than "leave it a warning". IMO warnings should only be used to temporarily downgrade an error in a project whilst in the process of migrating to the new pattern.

@72636c
Copy link
Member

@72636c 72636c commented Sep 21, 2020

I find @typescript-eslint/explicit-module-boundary-types draconian even outside of React, and it's sneakily disabled for skuba projects. I think I snuck that one in behind @etaoins though, who is the senior principal advocate for it

@etaoins
Copy link

@etaoins etaoins commented Sep 21, 2020

I'm fine disabling @typescript-eslint/explicit-module-boundary-types for Sku. It's pretty full on.

@markdalgleish
Copy link
Member

@markdalgleish markdalgleish commented Sep 21, 2020

I vote to remove it, seems a bit heavy handed.

@jahredhope
Copy link
Member

@jahredhope jahredhope commented Sep 21, 2020

If skuba and sku both disable then lets disable in eslint-config-seek?

@mattcompiles
Copy link
Contributor Author

@mattcompiles mattcompiles commented Sep 21, 2020

PR updated to use eslint-config-seek@7.0.5

@mattcompiles
Copy link
Contributor Author

@mattcompiles mattcompiles commented Sep 22, 2020

This PR is ready to go now. CI finally passing.

@72636c
72636c approved these changes Sep 22, 2020
Copy link
Member

@72636c 72636c left a comment

🤩

@mattcompiles mattcompiles merged commit 51bb25c into master Sep 22, 2020
1 check passed
1 check passed
Lint & Test
Details
@mattcompiles mattcompiles deleted the typescript-4 branch Sep 22, 2020
@seek-oss-ci seek-oss-ci mentioned this pull request Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.