Skip to content

Pass typescript options via query string #141

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 2 commits into from
Apr 13, 2017

Conversation

jvanbruegge
Copy link
Contributor

This allows you to pass options not only via tsconfig.json but also via webpack.config.js
I would need this for cyclejs-community/create-cycle-app#103

@jvanbruegge
Copy link
Contributor Author

I tested it locally and it works as expected

@jvanbruegge jvanbruegge force-pushed the ts-options branch 2 times, most recently from 44ab877 to f4c3c97 Compare April 10, 2017 16:20
@andywer
Copy link
Owner

andywer commented Apr 10, 2017

Good point. And as always thanks for the PR :)

But tsconfig and compiler options will now be passed as JSON options and via TsConfigPathsPlugin. Maybe it doesn't do any harm, but doesn't seem completely right either. Maybe separate the TsConfigPathsPlugin options from the loader options in the beginning?

@jvanbruegge
Copy link
Contributor Author

I missunderstood the plugin, this is better now

@andywer
Copy link
Owner

andywer commented Apr 11, 2017

Ok, now I am confused ^^
I can't find any advancedPath option on https://github.com/s-panferov/awesome-typescript-loader#loader-options

@jvanbruegge
Copy link
Contributor Author

yes, because normally you would either add the plugin or not, the option just handles inclusion

@andywer
Copy link
Owner

andywer commented Apr 11, 2017

Hmm, I like the proposed change in general, but I don't like having an additional custom option for utility purposes only. Maybe options.paths for tsconfig and compiler, the rest of options becomes loader options.

What do you say?

@jvanbruegge
Copy link
Contributor Author

the loader needs the compiler and tsconfig too

@jvanbruegge
Copy link
Contributor Author

i never used this feature (and i guess noone else too), so this never surfaced

@andywer
Copy link
Owner

andywer commented Apr 11, 2017

But what is the TsConfigPathsPlugin good for if we have to pass the paths to the loader anyway?

Just recognized you were the autor of the block, so in case of doubt I tend to trust what you say 😉
But it still seems a little strange.

@jvanbruegge
Copy link
Contributor Author

Im not completely sure either, the docs dont say why

@andywer
Copy link
Owner

andywer commented Apr 11, 2017

Maybe it's worth opening an issue at the awesome-typescript-loader's repo?

@jvanbruegge
Copy link
Contributor Author

yes i can do that.

@jvanbruegge
Copy link
Contributor Author

There is already one, but it seems I use it right now

@andywer
Copy link
Owner

andywer commented Apr 12, 2017

How about dropping the options.advancedPaths and go back to the options.tsconfig || options.compiler condition for the TsConfigPathsPlugin if we need it plus the complete JSON config?

(Especially since @param {boolean} [options.advancedPath] See https://github.com/s-panferov/awesome-typescript-loader#loader-options seems misleading and the documentation for compiler & tsconfig has been dropped)

The rest looks fine to me :)

@jvanbruegge
Copy link
Contributor Author

I finally understood what the plugin is for. We can have it active all the time. It only affects people configuring it in the tsconfig.json. Thats why it needs the path. I will remove the check

@jvanbruegge
Copy link
Contributor Author

Should I remove the unit test? It's actually quite pointless

@andywer
Copy link
Owner

andywer commented Apr 12, 2017

Nice! Yes, I think it's safe to remove the unit tests. End-to-end tests are way more reliable for testing blocks. Poking #91...

But adding a comment explaining why we need the TsConfigPathsPlugin and why it's fine to feed it with null/undefined would be helpful.

@jvanbruegge
Copy link
Contributor Author

Can we merge this?

@andywer
Copy link
Owner

andywer commented Apr 13, 2017

Of course 👌

@andywer andywer merged commit 689a4da into andywer:master Apr 13, 2017
@jvanbruegge jvanbruegge deleted the ts-options branch April 13, 2017 15:18
@andywer
Copy link
Owner

andywer commented Apr 13, 2017

Published as @webpack-blocks/typescript v0.4.1

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