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

refactor: Use chalk instead of colors #579

Merged
merged 11 commits into from
Jul 12, 2017
Merged

Conversation

develar
Copy link
Contributor

@develar develar commented Jul 6, 2017

Close #62

Please note — yarn.lock is added since Yarn is much better than npm in terms of speed&reliability (#369).
In general, I recommend to remove package.lock from this repo and use only yarn, since no reasons to use npm, but anyway for now this PR contains modified package.lock (because it is npm, file changes is very big due to npm 5.1 release).

* to reduce dependency tree, because most of the modern packages uses chalk, not colors
* Doesn't extend String.prototype and https://github.com/chalk/chalk#highlights

Close TypeStrong#62
@develar
Copy link
Contributor Author

develar commented Jul 6, 2017

CI failures are not related to my changes, as far I see.

@johnnyreilly
Copy link
Member

Hi @develar,

First of all, thanks for sending this! I won't have be able to look at this immediately but I will get to it.

The CI failures are related to your change. Essentially the comparison test pack is kind of brutal and literally compares output to output. The result of your changes has changed the line number of stack traces; see below:

  1) nodeModulesMeaningfulErrorWhenImportingTs should have the correct output:
      Uncaught AssertionError: output.txt is different between actual and expected
      + expected - actual
       Module build failed: Error: Typescript emitted no output for node_modules/a/index.ts.
       You should not need to recompile .ts files in node_modules.
       Please contact the package author to advise them to use --declaration --outDir.
       More https://github.com/Microsoft/TypeScript/issues/12358
      -    at Object.loader (dist/index.js:31:15)
      +    at Object.loader (dist/index.js:32:15)

The line number is now 32 rather than 31. Arguably the test pack should ignore this; it's not that relevant. We ignore other differences such as number of bytes etc.

If you'd like to either regenerate the output for those 3 tests / patch it for new line numbers or make the comparison test pack ignore the line numbers in the stack trace then we'll be green.

@johnnyreilly
Copy link
Member

Hi @develar,

I did some work at the weekend to get the tests passing. I'm pretty happy with this; I just want to do some local experimentation and then I'll look to merge. Thanks again!

@develar
Copy link
Contributor Author

develar commented Jul 11, 2017

@johnnyreilly Thanks for you work, sorry, that I didn't start to fix it on weekend, was busy on another open source project.

@johnnyreilly
Copy link
Member

Merging - thanks for your work! This will go out with the next release of ts-loader.

@johnnyreilly johnnyreilly merged commit 7b447c6 into TypeStrong:master Jul 12, 2017
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.

2 participants