Skip to content

add documentation for appendTsSuffixTo #359

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

Merged
merged 1 commit into from
Nov 7, 2016
Merged

add documentation for appendTsSuffixTo #359

merged 1 commit into from
Nov 7, 2016

Conversation

HerringtonDarkholme
Copy link
Contributor

Sorry for the delay, get quite busy these days.

@johnnyreilly
Copy link
Member

No problem - thanks for this! My machine has just died on me but once I get it up and running again I'll look at cutting a new release

@johnnyreilly johnnyreilly merged commit 229053a into TypeStrong:master Nov 7, 2016
@johnnyreilly
Copy link
Member

BTW I managed to get a fix into our comparison test pack to ignore the data-v-ab12etc hot module hashing when performing comparisons. That's great. However it uncovered what looks like a potential issue for people on Windows using ts-loader for vuejs. On Windows absolute paths seem to be being used rather than relative ones. Do you have any ideas about this? I'll do some more digging... (Still need to commit the change to the test pack as well....)

@HerringtonDarkholme
Copy link
Contributor Author

HerringtonDarkholme commented Nov 7, 2016

I would suggest this might be handled by vue-ts-loader, which I want to migrate to be a preconfigured ts-loader. It is really generous for ts-loader to support appendTsSuffix. But handling vue-specific quirks should be the task for vue-ts-loader.

Also, I think vuejs would use absolute paths every where(regardless of path-separator), I just manually deleted the absolute path in output to avoid personal information leakage...:(

@johnnyreilly
Copy link
Member

I just manually deleted the absolute path in output to avoid personal information leakage...:(

Ah! That makes sense 😄 I'll have a ponder about this.

handling vue-specific quirks should be the task for vue-ts-loader.

Agreed. Does appendTsSuffix actually give you something useful then? If you're not planning to make use of it then I'm pondering if it should be part of ts-loader. I'm totally happy for it to be but if it won't be used then I'm a little hesitant to add it.

@HerringtonDarkholme
Copy link
Contributor Author

Does appendTsSuffix actually give you something useful then?

Very useful. vue-ts-loader will have to fork a copy of ts-loader if this option is not added. I can handle vue's path looking, export format or what ever vue specific in other loader. But appending suffix is simply mission impossible if no change is made underlying ts-loader.

I would even think this might be useful for hash bang file. TypeStrong/ts-node#116

@johnnyreilly
Copy link
Member

Very useful.

Then it will ship 🚢 😄

So is the idea that vue-ts-loader will be chained with ts-loader or will vue-ts-loader wrap ts-loader?

Just so you know, the longer term aim is to join together awesome-typescript-loader and ts-loader. The initial steps of that will be ensuring both loaders support the same options. So if you need appendTsSuffix we want to make sure it remains supported.

@HerringtonDarkholme
Copy link
Contributor Author

HerringtonDarkholme commented Nov 7, 2016

I could only implore you to keep this option. Frankly, no many users use TS + Vue. vue-ts-loader merely have 30 stars on github. And on npm only 200+ downloads last month. As a comparison, ts-loader has 294,548 downloads last month.

Yet the overhead added to ts-loader is relatively small (adding suffix if filename matches) and would hardly break other plugins ( except vue-loader), and this is the only loader I could find to support ts in vue file. Reference: microsoft/TypeScript#10939 non-ts extension would hardly be supported.

However, I can foresee that with the combination of ts-loader and ATL the code base will grow larger and many new features will be added. If the cost for this option is no longer small, I'm fine to remove it.

@johnnyreilly
Copy link
Member

As you say:

the overhead added to ts-loader is relatively small

So as it stands I think it's fine and we'll include it. If there's some reason in the future where it becomes problematic we'll certainly discuss it before taking any action. But I'd imagine that this can carry on being supported.

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