Skip to content

Conversation

@misterdev
Copy link
Contributor

@misterdev misterdev commented Feb 19, 2019

What kind of change does this PR introduce?
Includes changes requested in #613

Summary

Before committing I had to solve those issues:

  • Removed deprecated lang: [0, "always", "eng"] from commitlint configuration. It gave me errors preventing me from committing:

    RangeError: Found invalid rule names: lang. Supported rule names are: body-case, body-empty

  • Fixed a tslint error:

 ERROR: packages/migrate/index.ts:20:4 - Type literal has only a call signature — use 
`new(errors: string[]) => {
    message: string;
},` instead.

Those are the actual changes:

  1. Adds an interface to transformations in packages/migrate/migrate.ts
  2. Refactor every test in migrate/**/__tests__/**.test.ts
//before
defineTest(join(__dirname, ".."), "resolve");

//after
const dirName: string = path.join(__dirname, "..");
defineTest(dirName, "resolve");
  1. Adds a type notation in packages/utils/__tests__/ast-utils.test.ts

  2. I've also refactored packages/migrate/resolve/__tests__/__testfixtures__/resolve.input.js following this comment.
    There's something I don't understand well, changing const path = require("path") with const {resolve} = require("path") gives me an error:

screenshot 2019-02-19 at 20 38 23

Why does this happens?

Feedbacks are welcome :)

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@misterdev misterdev changed the title [WIP] tests(migration): including requested changes tests(migration): including requested changes Feb 19, 2019
@ematipico
Copy link
Contributor

If you inside a typescript file, I discorage the use of require if possible

@misterdev
Copy link
Contributor Author

Thanks for reviewing!
The only require I see are in resolve.input.js that's a js file and in resolve.test.ts.snap that I think is a jest snapshot

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

For me it looks ok, lgtm. 💚

@evenstensberg evenstensberg merged commit 210ef72 into webpack:ts-test Mar 3, 2019
@misterdev misterdev deleted the misterdev-ts-test-fixreview branch May 5, 2019 10:32
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