add appendTsxSuffixTo option #581

vhqtvn · 2017-07-12T18:00:34Z

Add support for tsx.

johnnyreilly · 2017-07-12T18:50:05Z

Thanks for this! Could you tell me what the use case for this is? Also please could you fix the tests and provide a new one for the new feature? Thanks!

vhqtvn · 2017-07-12T20:56:12Z

Hi, this allow us to use options like this:

                        options: {
                            appendTsSuffixTo: [/\.vue$/],
                            appendTsxSuffixTo: [/\.vuex$/],
                        },

So we can use jsx in *.vuex:

<script lang="tsx" type="text/typescript">
import Vue from 'vue';
const MyElm = {
    functional: true,
    render (h: Vue.CreateElement, context: Vue.RenderContext) {
        return (<div>Hello world!</div>);
    }
}
...
</script>

johnnyreilly · 2017-07-12T21:11:48Z

src/index.ts

@@ -27,7 +27,8 @@ function loader(this: interfaces.Webpack, contents: string) {
    }

    const rawFilePath = path.normalize(this.resourcePath);
-    const filePath = utils.appendTsSuffixIfMatch(options.appendTsSuffixTo, rawFilePath);
+    let filePath = utils.appendTsSuffixIfMatch(options.appendTsSuffixTo, rawFilePath);


Come the first uses rawFilePath and the second uses filePath?

So why this:

let filePath = utils.appendTsSuffixIfMatch(options.appendTsSuffixTo, rawFilePath); filePath = utils.appendTsxSuffixIfMatch(options.appendTsxSuffixTo, filePath); // shouldn't this be rawFilePath too?

The only different is when filename is matched by both options.appendTsxSuffixTo and options.appendTsSuffixTo.
In that case, if both part use rawFilePath then .tsx append will overwrite .ts, but i think it should become .ts.tsx so user may know that their regexes has collision.

Right - thanks for that. Is it likely that people will use appendTsxSuffixTo and appendTsSuffixTo together? Or is it an either / or thing?

They can use it together if they want (declare at a single place and then reuse for example), i can code it more generic but after consider backward-compability i chose this method.

vhqtvn · 2017-07-13T07:17:33Z

Hi @johnnyreilly Please help me on the failed test of travis-ci, seems to be a nodejs problem?

johnnyreilly · 2017-07-13T08:19:48Z

I spy green tests - looks like you're getting there!

vhqtvn · 2017-07-13T08:21:09Z

I updated documents only to trigger re-test :( the CI's are stupid.

johnnyreilly · 2017-07-13T08:23:31Z

Yeah - the integration tests are somewhat flaky which is a pain. Way too many false negatives.

johnnyreilly · 2017-07-13T08:40:31Z

README.md

+    entry: './index.vue',
+    output: { filename: 'bundle.js' },
+    resolve: {
+        extensions: ['.ts', '.vue']


Does '.vuex' need to be here as well?

Yes i'm sorry, i committed a new one.

johnnyreilly · 2017-07-13T08:56:11Z

src/utils.ts

@@ -88,6 +88,17 @@ export function appendTsSuffixIfMatch(patterns: RegExp[], path: string): string
    return path;
 }

+export function appendTsxSuffixIfMatch(patterns: RegExp[], path: string): string {


Having pondered I think it might make for nicer code to combine appendTsSuffixIfMatch and appendTsxSuffixIfMatch.

Here's my reasoning:

Wherever appendTsSuffixIfMatch is called in the code, the output is then passed into the call to appendTsxSuffixIfMatch. They are never called in isolation and are intended to be used together.

That being the case, I think it might make sense to have an appendTsTsxSuffixIfMatch method which internally does the appendTsSuffixIfMatch and appendTsxSuffixIfMatch operations, feeding the output of the first into the second. This should result in clearer code at the call sites. It may also allow for some genericisation for the underlying methods (whether or not that makes sense I don't know)

Is that okay with you? I'm quite focused on keeping the source of ts-loader as comprehensible as possible to ease contributions by others.

I've just commit.

vhqtvn · 2017-07-13T20:00:01Z

hi @johnnyreilly all tests passed :) please review and merge, thank you!

johnnyreilly · 2017-07-13T20:08:19Z

src/index.ts

@@ -26,7 +26,8 @@ function loader(this: interfaces.Webpack, contents: string) {
    }

    const rawFilePath = path.normalize(this.resourcePath);
-    const filePath = utils.appendTsSuffixIfMatch(options.appendTsSuffixTo, rawFilePath);
+    let filePath = utils.appendSuffixIfMatch(options.appendTsSuffixTo, rawFilePath, '.ts');
+    filePath = utils.appendSuffixIfMatch(options.appendTsxSuffixTo, filePath, '.tsx');
    const fileVersion = updateFileInCache(filePath, contents, instance);


Could this be appendSuffixesIfMatch also please? Thanks!

I'm sorry i already code that but forgot to save...

johnnyreilly · 2017-07-13T20:29:25Z

We've all been there 👍

johnnyreilly · 2017-07-14T04:43:03Z

The test is failing because of hyper-sensitivity on the part of the comparison test pack. Actually I've fixed that in my latest commit and so if you sync up we should get green tests. Want to give it a go? Instructions here (if you need them): https://help.github.com/articles/syncing-a-fork/

Also @HerringtonDarkholme are you happy with this PR? I ask since you originally added vue support to ts-loader...

HerringtonDarkholme · 2017-07-14T05:04:33Z

If the only use case is using TSX in vue's SFC. I would not think this is an appropriate approach.
First, vuex isn't a file extension supported officially, other tools like vetur do not identify it. Second, vue's script type is identified by "lang" attribute, not file extension.

The most important issue is vue's tsx ecosystem. See vuejs/vetur#325 (comment) for more detail.

johnnyreilly · 2017-07-14T05:10:13Z

@vhqtvn could you respond to @HerringtonDarkholme when you get a chance? Thanks.

vhqtvn · 2017-07-14T05:17:43Z

@HerringtonDarkholme vuex is just my attempt to use tsx for my project with currently fixed structure setup, the regex can be used like /\/src\/vue-tsx\// or any kind of selector to indicate the target vue file use tsx. Current appendTsSuffixTo work very well for ts, and so will Tsx. I do understand tsx has limits but it is still very useful (and useable) for functional components and is currently working in my project.

vhqtvn · 2017-07-14T08:49:01Z

You can also contribute to vue-ts-loader which is dedicated to type check vue file....

Again this is still a choosing problem, one can prefer dedicated tools to solve specified problems but one prefer using generic tools to provide more flexible configurations to the project, as long as it's maintainable by them.

If you cannot make Vue's JSX work in a .tsx file ...

We can, as long as build tool can handle JSX and TS preserve JSX to the building toolchain "jsx": "preserve",.

HerringtonDarkholme · 2017-07-14T08:51:58Z

We can, as long as build tool can handle JSX and TS preserve JSX to the building toolchain "jsx": "preserve",.

No. The core aim of TS is to type check. Without type checking Vue JSX one cannot say Vue JSX works. For comparison, React's JSX works out of box. Type checking an completion work fine.

one can prefer dedicated tools to solve specified problems but one prefer using generic tools

appendTSSuffix sounds like a generic option but sadly it is only used by Vue. I'm quite sorry for introducing this to ts-loader but at the end it attracts many Vue users to ts-loader so I would argue it has significant advantage. One need to justify tsx would attract new users.

Besides advantages, Vue support in ts-loader is pretty much nothing but hack. It requires vue-loader to read all vue file before ts-loader kicks in. So you have constraints to use ts-loader + vue, for example, the entry file must include or recursively include all vue files. It is ideologically atrocious implementation. Yet it enables type checking in Vue, a trade off for significant usage.

If plugin is supported, it can be supported ideally.

vhqtvn · 2017-07-14T08:56:05Z

Without type checking Vue JSX one cannot say Vue JSX works.

I think you mean Without type checking Vue JSX one cannot say Vue TSX works, but tsx here provide solutions for target users that need to use JSX for rendering function and still use TS for the rest, that's too messy to split out logics to standalone tsx file for every component that need it.

HerringtonDarkholme · 2017-07-14T09:08:02Z

target users that need to use JSX for rendering function

I have demonstrated in length that it is a bogus requirement. You already have render function and template in Vue. If you do need JSX for functional component consider extracting it to external template. If you mix JSX and component template, you'd better refactor for cleaner separation.

appendTSXSuffix isn't a good fix. I have said that Vue uses lang attribute not vuex. It's nonstandard. Waiting is better than bike shedding.

https://blog.golang.org/toward-go2

In contrast, adding new API to Go would add new problems:

We did what we always do when there's a problem without a clear solution: we waited. Waiting gives us more time to add experience and understanding of the problem and also more time to find a good solution.

vhqtvn · 2017-07-14T09:15:31Z

I have said that Vue uses lang attribute not vuex

We dont need vuex, it's just my personal way, we can use tsx in vue too as long as the regex match.

johnnyreilly · 2017-07-14T09:17:39Z

Hi @HerringtonDarkholme and @vhqtvn,

First of all, thanks both for your contributions - they're massively valuable and well done for keeping it rational. ❤️

Here's where I am:

I don't know Vue (at all) but looking at the docs it seems to have some concept of JSX: https://vuejs.org/v2/guide/render-function.html#JSX. This makes me think that it might be reasonable to allow support for TSX. However, it's not clear to me that React JSX === Vue JSX. I think @HerringtonDarkholme is essentially saying that React JSX !== Vue JSX and so there is no real concept of type checking.

Is that right? In that case, is there any value in adding this?

I have said that Vue uses lang attribute not vuex. It's nonstandard. Waiting is better than bike shedding.

I'm not clear about the whole vuex / lang thing here. (Again, not a Vue user.) Can anyone explain to me what you mean? Speak as you might to a young child. Or a golden retriever. (Couldn't resist the Margin Call hat tip 😉 ) Honestly I've followed only about 30% of the Vue-specific conversation above.

Finally, the fact that so many people have chimed in and said they want this makes me think that there's something valuable here. I don't want to ignore that - but I don't want to implement something that's subject to change in 10 minutes time. Is it?

Does this feature enable standard Vue usage? Or is it a custom approach that's quite popular? Both are legitimate but if this is custom I wonder if it's appropriate baking it in to ts-loader. If that's the case, would this suggestion resolve the issue in any way:

You can also contribute to vue-ts-loader which is dedicated to type check vue file

?

I'm totally on the fence right now.

vhqtvn · 2017-07-14T09:29:55Z

Does this feature enable standard Vue usage? Or is it a custom approach that's quite popular? Both are legitimate but if this is custom I wonder if it's appropriate baking it in to ts-loader.

Yes it enables standard Vue usage because Vue already support JSX - with some limits of cause due to the different between Vue and React.
Despite to these limits there're still users that need JSX, so offical vue also supports it.
This JSX approach is also generic if we do not look into Vue's space, with the same purpose as appendTsSuffix, the usage is up to user to use typescript (because the compiler require correct extension) the right way to solve their problem.

HerringtonDarkholme · 2017-07-14T11:39:26Z

I have tried tsx with Vue locally. Actually I think this option brings more problems than it solves.

It is very hard to use tsx with strict / noImplicitAny. This is not acceptable to me.
As mentioned above, users cannot mix ts/tsx within vue unless using some other non-standard extensions.

These two problems will cause more issues to Vue/TypeScript/ts-loader's repos.

vhqtvn · 2017-07-14T11:45:44Z

It is very hard to use tsx with strict / noImplicitAny. This is not acceptable to me.

Please explain this more clear, what's the problems you came with?

As mentioned above, users cannot mix ts/tsx within vue unless using some other non-standard extensions.

Ts can be used in vue with standard extension (i.e. https://github.com/vuejs/vue-class-component), Tsx is the better way to code render function within Ts, and it use no more than vuejs/babel-plugin-transform-vue-jsx and ts-loader.

HerringtonDarkholme · 2017-07-14T11:49:21Z

Open a new ts project and set strict to true. Install VueJS and write tsx. Try it. If you can make it compile I think we can merge this pull request.
I mean, mixing. In one vue file I use lang="ts" and <any> castVar. In another vue file I use lang="tsx" and <div></div>. You solution does not cover this.

rhyek · 2017-07-14T16:43:31Z

About the createElement vs JSX thing: there is no way I or anyone on my team is going to write a complex 100+ line template using createElement. Readability would be non-existant. I've always interpreted createElement as something other things will always compile to, not something I want to personally write.

vhqtvn · 2017-07-14T16:55:39Z

@HerringtonDarkholme https://github.com/vhqtvn/test-vue-tsx

HerringtonDarkholme · 2017-07-14T17:05:23Z

@vhqtvn Do you ever know how to correctly set up tsconfig? strict should be inside "compilerOptioins"

johnnyreilly · 2017-07-14T17:10:23Z

Hey @HerringtonDarkholme,

I know you feel strongly but let's not move away from courteous disagreement to abuse. ts-loader has got this far powered by ❤️ Let's be good to each other!

rhyek · 2017-07-14T18:20:37Z

FYI, regarding a TS language service plugin - from https://github.com/Microsoft/TypeScript/wiki/Writing-a-Language-Service-Plugin:

TypeScript Language Service Plugins ("plugins") are for changing the editing experience only. The core TypeScript language remains the same. Plugins can't add new language features such as new syntax or different typechecking behavior, and plugins aren't loaded during normal commandline typechecking or emitting.

So, it wouldn't be possible to have a plugin transform JSX to vue's createElement. Babel (or something similar) would be required here.

vhqtvn · 2017-07-14T18:44:52Z

Hi all, i updated the example repo, strict mode wont check JSX typings but all remain functions of ts work, but that's enough for us to write JSX templates within ts code. @HerringtonDarkholme

octref · 2017-07-14T19:02:27Z

Please no invention like .vuex.
@johnnyreilly Vuex is Vue's redux: https://vuex.vuejs.org/. It's an official library from Vue: https://github.com/vuejs/vuex
Such extension is going to confuse the hell out of people.

vhqtvn · 2017-07-14T19:04:49Z

@octref .vuex is a personal choice and have nothing to do with this pull request.

jbrantly · 2017-07-14T19:17:57Z

Disclaimer: I'm not a Vue user and I like JSX quite a bit

@HerringtonDarkholme

It seems clear to me that you don't like JSX/TSX, which is fine, but I'm having a hard time separating your dislike of the syntax from the logical rationale for not including this PR for those folks who do like it. I think I can distill your arguments into the following (hopefully?):

(J|T)SX has various shortcomings in Vue (strict mode, event handlers, babel setup, IDE support, etc)
lang should be used to determine TS vs TSX, not a .vuex or other file extension
The majority of ts-loader users don't care or need this feature

My reactions would be:

I don't think this is a reason to not support something, especially if we think it will be supported better in the future. I tend to agree there is a bit of a chicken and egg problem here. Also, most of the complexity here is not in ts-loader-land but needs to be solved elsewhere. The support that would need to come from ts-loader seems fairly minimal.
Not a Vue user but it does seem like for SFC files the lang attribute is the appropriate way to identify what something is. I think a proper implementation of this should definitely be to use the lang attribute it at all possible and not file extensions.
Again, I don't think this is necessarily a reason to not support something. "Chicken and egg", "if you build it they will come", etc.

I have a couple of questions though:

A. I was looking at https://github.com/Microsoft/TypeScript-Vue-Starter and for the life of me I can't figure out how that configuration works with SFC. In the case of SFC, doesn't vue-loader need to be explicitly configured to call ts-loader? I'm assuming that's the case and the example there is just wrong.
B. When vue-loader calls ts-loader with the contents of the <script /> tag what information can be gleaned from it? For example, is it possible to know whether lang="ts" vs lang="tsx" was specified?
C. Assuming that you can't determine the contents of the lang tag from (B), I'm wondering if we couldn't do a vue-loader configuration like so:

loaders: {
  'ts': 'ts-loader',
  'tsx': 'ts-loader?appendTsx'
}

For that matter, might it not be possible to get rid of "appendTsSuffixTo" and have something like:

loaders: {
  'ts': 'ts-loader?append=ts',
  'tsx': 'ts-loader?append=tsx'
}

I'm mostly thinking out loud here, there might be technical issues with actually implementing those since TypeScript is super-eager to load all files off disk.

vhqtvn · 2017-07-14T19:37:00Z

@jbrantly
A. B. Yes vue-loader call ts-loader, i think it's a good way.
C. I already think about these, but then decided to keep thing backward-compatible so the pull wont affect existing users.

johnnyreilly · 2017-07-14T20:33:43Z

Please no invention like .vuex.
@johnnyreilly Vuex is Vue's redux: https://vuex.vuejs.org/. It's an official library from Vue: https://github.com/vuejs/vuex
Such extension is going to confuse the hell out of people.

That's a valid point @octref - if we look to merge this then I'm sure @vhqtvn would be open to changing the docs example to something other than vuex to avoid confusion.

@jbrantly - thanks so much for commenting. As ever, you make some very useful points. What you've said makes sense and brings some useful clarity to the discussion. 👍

Here's my thinking at present:

We have appendTsSuffixTo which allows people to take non-ts files and treat them as ts files. Given TypeScript has 2 language "modes" if you will (ts and tsx) and we're already supporting one on the file-suffix front, it seems reasonable to have the other language mode supported as well. Even if you think it's terrible - the horse has already bolted. We're already half in, why not be all in? appendTsxSuffixTo is just the logical sibling of appendTsSuffixTo
In terms of complexity this actually doesn't add much to ts-loader. We're essentially just taking what we're already doing for appendTsSuffixTo and saying "once more with feeling". It's not a significant change to ts-loader code-wise.
If people don't use this feature of ts-loader (as I won't) it doesn't affect them. It's there and the way it's implemented is suitably generic. It's in no way tied to Vue itself thanks to solid design choices by @HerringtonDarkholme when appendTsSuffixTo was first implemented. If a future version of Vue changes significantly it has no bearing on the feature we're proposing to add to ts-loader.

@HerringtonDarkholme is there anything in my thinking that you disagree with? Anything I've missed? I understand that you're not that keen on appendTsSuffixTo anymore and that's fine. But putting personal taste aside is there a significant problem this PR might create? Your contributions to ts-loader have been really valuable and I'm really interested to know of any real harm you think this PR might do. I value your opinion.

…pendTsxSuffixTo has been passed

HerringtonDarkholme

Just merge this since @vhqtvn has make tsx to compile.

But warning ahead, maintaining a tool is more than maintaining code. Vue TSX has more hairy complication than a simple appendSuffixTo can solve.

johnnyreilly · 2017-07-15T08:32:28Z

Thanks for that @HerringtonDarkholme - I appreciate you responding.

But warning ahead, maintaining a tool is more than maintaining code. Vue TSX has more hairy complication than a simple appendSuffixTo can solve.

I'm sure you're correct. I'm going to take this PR on the grounds that ts-loader won't be doing anymore than appending file suffixes to paths. That's minimal enough and it's agnostic to Vue. That's enough for me to be content.

I've made a tweak to ensure that the code doesn't get invoked unless the relevant options are passed - once the CI goes green I'll merge.

johnnyreilly · 2017-07-15T12:08:52Z

This shipped with ts-loader 2.3.0 - thanks everyone for contributing to the discussion. https://github.com/TypeStrong/ts-loader/releases/tag/v2.3.0

HerringtonDarkholme · 2017-07-17T08:55:01Z

Some framework support for VueJSX. I hope some on would have first made feature request to library before modifying build tools.

https://github.com/HerringtonDarkholme/av-ts#tsx-example

I would, again, argue that framework and language support should precede building tools. Premature releases only ships broken tools and harms user experience. Patience is the virtue.

johnnyreilly · 2017-07-17T09:22:26Z

That's a fair point and I don't disagree with you. In the end, the simplicity of the addition from a ts-loader perspective and the clear usefulness to a number of people made it a reasonable PR in my view.

vanhoavn added 2 commits July 13, 2017 00:55

add tsx option

9c3753a

update readme

694662a

update

7042560

johnnyreilly reviewed Jul 12, 2017

View reviewed changes

vanhoavn and others added 4 commits July 13, 2017 04:25

update test

e0fb71a

Merge branch 'master' into master

5afa608

update test for ts 2.4

e6efd1e

Merge branch 'master' of github.com:vhqtvn/ts-loader

90b66b8

vanhoavn added 2 commits July 13, 2017 14:32

add jsx example

61e3464

update README

9f606c7

johnnyreilly reviewed Jul 13, 2017

View reviewed changes

update example

5b11cae

johnnyreilly reviewed Jul 13, 2017

View reviewed changes

vanhoavn added 2 commits July 13, 2017 15:59

combine suffix append functions

e19b1a3

combine function usage

07ffb08

johnnyreilly reviewed Jul 13, 2017

View reviewed changes

update index.ts

d841e82

johnnyreilly requested a review from HerringtonDarkholme July 14, 2017 04:43

vanhoavn and others added 2 commits July 15, 2017 04:03

update readme

4a0fca6

appendSuffixesIfMatch is only invoked if appendTsSuffixTo and / or ap…

3a1c6a6

…pendTsxSuffixTo has been passed

HerringtonDarkholme approved these changes Jul 15, 2017

View reviewed changes

HerringtonDarkholme mentioned this pull request Jul 15, 2017

JSX does not work with lang=ts vuejs/vetur#325

Closed

2 tasks

johnnyreilly merged commit 80e0349 into TypeStrong:master Jul 15, 2017

add appendTsxSuffixTo option #581

add appendTsxSuffixTo option #581

Conversation

vhqtvn commented Jul 12, 2017

johnnyreilly commented Jul 12, 2017

vhqtvn commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vhqtvn commented Jul 13, 2017

johnnyreilly commented Jul 13, 2017

vhqtvn commented Jul 13, 2017

johnnyreilly commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vhqtvn commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyreilly commented Jul 13, 2017

johnnyreilly commented Jul 14, 2017 • edited Loading

HerringtonDarkholme commented Jul 14, 2017 • edited Loading

johnnyreilly commented Jul 14, 2017

vhqtvn commented Jul 14, 2017 • edited Loading

vhqtvn commented Jul 14, 2017

HerringtonDarkholme commented Jul 14, 2017 • edited Loading

vhqtvn commented Jul 14, 2017

HerringtonDarkholme commented Jul 14, 2017 • edited Loading

vhqtvn commented Jul 14, 2017

johnnyreilly commented Jul 14, 2017 • edited Loading

vhqtvn commented Jul 14, 2017

HerringtonDarkholme commented Jul 14, 2017

vhqtvn commented Jul 14, 2017

HerringtonDarkholme commented Jul 14, 2017

rhyek commented Jul 14, 2017

vhqtvn commented Jul 14, 2017

HerringtonDarkholme commented Jul 14, 2017

johnnyreilly commented Jul 14, 2017 • edited Loading

rhyek commented Jul 14, 2017

vhqtvn commented Jul 14, 2017 • edited Loading

octref commented Jul 14, 2017

vhqtvn commented Jul 14, 2017 • edited Loading

jbrantly commented Jul 14, 2017

vhqtvn commented Jul 14, 2017

johnnyreilly commented Jul 14, 2017

HerringtonDarkholme left a comment

Choose a reason for hiding this comment

johnnyreilly commented Jul 15, 2017 • edited Loading

johnnyreilly commented Jul 15, 2017

HerringtonDarkholme commented Jul 17, 2017

johnnyreilly commented Jul 17, 2017

johnnyreilly commented Jul 14, 2017 •

edited

Loading

HerringtonDarkholme commented Jul 14, 2017 •

edited

Loading

vhqtvn commented Jul 14, 2017 •

edited

Loading

HerringtonDarkholme commented Jul 14, 2017 •

edited

Loading

HerringtonDarkholme commented Jul 14, 2017 •

edited

Loading

johnnyreilly commented Jul 14, 2017 •

edited

Loading

johnnyreilly commented Jul 14, 2017 •

edited

Loading

vhqtvn commented Jul 14, 2017 •

edited

Loading

vhqtvn commented Jul 14, 2017 •

edited

Loading

johnnyreilly commented Jul 15, 2017 •

edited

Loading