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

Chore: Build packages/hint in strict mode #1356

Closed
wants to merge 31 commits into from

Conversation

antross
Copy link
Member

@antross antross commented Oct 1, 2018

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Sets "strict": true in package.json for packages/hint.
Builds, but branch coverage is currently below threshold for tests.
Also still chasing down breaks in dependent packages caused by this work.

import getFileExtension from './fs/file-extension';
import getFileName from './fs/filename';
import normalizeString from './misc/normalize-string';

const mimeDB = require('mime-db');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment here like in the other requires?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this one shouldn't have been changed as it doesn't fit the same pattern (it returns an object). It was leftover from a search for better types so I'll change it back since the modern syntax is fine for this one.

Copy link
Member

@molant molant left a comment

Choose a reason for hiding this comment

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

Awesome work @antross, can't wait to get this merged.
I just have one comment but everything else LGTM.

Thanks!

packages/hint/src/lib/@types/jsdom-lib-old-api.d.ts Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@
import { CLIOptions } from '../types';

/** Wrapper to dynamically load the different CLI tasks depending on a condition */
const action = (pkg: string, condition?: string): (actions: CLIOptions) => Promise<boolean> => {
const action = (pkg: string, condition?: keyof CLIOptions): (actions: CLIOptions) => Promise<boolean> => {
Copy link
Member

Choose a reason for hiding this comment

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

😍

@@ -388,7 +388,7 @@ export class Configuration {
}

// In case the user uses the --watch flag when running hint
if (actions && actions.watch) {
if (actions && actions.watch && userConfig.connector && userConfig.connector.options) {
Copy link
Member

Choose a reason for hiding this comment

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

🤞 that https://github.com/tc39/proposal-optional-chaining will advance stage 1 one day so we can avoid this type of code.

@molant
Copy link
Member

molant commented Oct 4, 2018

@antross I think you need to rebase from master.
@sarvaje @alrra can you please take a look at this PR as well?

Thanks!

Aligns the types with what was already supported in code.
This was breaking using strict in callers from other packages.
Will be re-introduced in a separate commit.
No longer restricts to `NamedNodeMap` of which most methods are
unused, but instead allows any object implementing the `length`
property with either an `item()` method or an index signature, the
latter of which supports `Array`s.
Aligns with `IConnectorConstructor`.
Aligns with `IConnectorConstructor`.
This intermittently fails - the complex method signature seems to
confuse the TypeScript compiler depending on some unknown
piece of state. Ok treating as `any` for tests.
@molant
Copy link
Member

molant commented Oct 4, 2018

@antross will you believe me if I tell you this branch has conflicts again? @alrra will you have time to review this today? If not I'll go ahead and merge it once they are solved.

Copy link
Contributor

@sarvaje sarvaje left a comment

Choose a reason for hiding this comment

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

LGTM

@alrra alrra closed this in a928fbe Oct 5, 2018
@alrra
Copy link
Contributor

alrra commented Oct 5, 2018

@antross Good job! 👏🏻

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.

None yet

4 participants