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

Add fileName as prefix to error message to support Visual Studio #356

Merged
merged 5 commits into from
Nov 10, 2016

Conversation

gamli
Copy link
Contributor

@gamli gamli commented Nov 3, 2016

This allows Visual Studio to search the output of ts-loader for errors and show them in its error list.
See https://msdn.microsoft.com/en-us/library/yxkt8b26.aspx for a description of the format.
The path is normalized so the platform's path separator is used (Visual Studio doesn't handle those forward slashes very well - i.e. jumping to the line number does not work).

This allows Visual Studio to search the output of ts-loader for errors and show them in its error list.
See https://msdn.microsoft.com/en-us/library/yxkt8b26.aspx for a description of the format.
The path is normalized so the platform's path separator is used (Visual Studio doesn't handle those forward slashes very well - i.e. jumping to the line number does not work).
@jbrantly
Copy link
Member

jbrantly commented Nov 3, 2016

I wonder if this should be behind a flag (not advocating that it should be, just thinking out loud). The existing format is really meant to be compatible with other webpack output, and having the file name twice seems kind of repetitive. On the other hand, having the output display in Visual Studio seems useful for those who use it.

@gamli
Copy link
Contributor Author

gamli commented Nov 3, 2016

Yes, this would be better if someone attempts to parse the exact output. But I am not sure how the tests could be modified to fit this behaviour.

@johnnyreilly
Copy link
Member

Ah - yes I think I misunderstood your original issue. I'd say that I agree with @jbrantly that if this goes in it should definitely behind a flag as for the general use case as we'd want ts-loader to remain consistent with WebPack.

Would you be willing to amend your PR to put this behind a flag called something like visualStudioErrorFormat or something? We'd want a single new test which used this new flag and thus expected error text in the VS format. I'd advocate cloning one of the existing error tests.

@johnnyreilly
Copy link
Member

If you're after a good candidate aliasResolution would probably do the job if you removed the patch0 and commonComponents subdirectories.

@gamli
Copy link
Contributor Author

gamli commented Nov 3, 2016

Yes sure - I will be making that change tomorrow morning. It feels a lot better to just add a test/feature than breaking existsing stuff.

Just for giggles:
Today I spent about 2 hours changing the 63 files with expected output by hand - struggling with invisible ESC characters for changing colors and lack of knowledge of the inner workings of npm colors.
That was, until I realized that there is the option "--save-output" :)

@johnnyreilly
Copy link
Member

Today I spent about 2 hours changing the 63 files with expected output by hand - struggling with invisible ESC characters for changing colors and lack of knowledge of the inner workings of npm colors.
That was, until I realized that there is the option "--save-output" :)

OUCH 😄

mdibo added 2 commits November 10, 2016 11:42
See GitHub PR TypeStrong#356: Revert changes to test cases as preparation to make Visual Studio output configurable
@gamli
Copy link
Contributor Author

gamli commented Nov 10, 2016

Sorry for the delay, but I got sick on friday.

I implemented the changes:

  • Turn on/off Visual Studio error output with the flag visualStudioErrorFormat
  • Add test visualStudioErrorFormat (based on aliasResolution but without patch0)

I am not quite sure if the tests with output.transpiled.txt are necessary.

@johnnyreilly
Copy link
Member

Cool - could you update the documentation in the readme with the new option please?

@gamli
Copy link
Contributor Author

gamli commented Nov 10, 2016

Done.

@johnnyreilly
Copy link
Member

Thanks!

@johnnyreilly johnnyreilly merged commit be240b4 into TypeStrong:master Nov 10, 2016
@gamli
Copy link
Contributor Author

gamli commented Nov 10, 2016

One more question: is it possible to reference the latest version through npm or do i have to reference my fork?

@johnnyreilly
Copy link
Member

You can reference ts-loader on GitHub now your fork has been merged in. It's not been published to npm yet.

@gamli
Copy link
Contributor Author

gamli commented Nov 10, 2016

Ok, thanks!

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.

3 participants