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 new loader option "contextAsConfigBasePath", which when set to tr… #681

Merged
merged 4 commits into from
Nov 29, 2017
Merged

Add new loader option "contextAsConfigBasePath", which when set to tr… #681

merged 4 commits into from
Nov 29, 2017

Conversation

christiantinauer
Copy link
Contributor

@christiantinauer christiantinauer commented Nov 17, 2017

…ue, parses to tsconfig file with respect to webpack context path (https://webpack.js.org/configuration/entry-context/#context) instead of the location of the config file. So all relative paths in config file are treated relative to webpack context. This makes configurations possible, where the tsconfig file is located in a npm package or elsewhere in the project structure.

…ue, parses to tsconfig file with respect to webpack context path (https://webpack.js.org/configuration/entry-context/#context) instead of the location of  the config file. So all relative paths in config file are treated relative to webpack context This makes configurations possible, where the tsconfig file is located in a npm package or elsewhere in the project structure.
@johnnyreilly
Copy link
Member

Thanks for this! Please could you provide a test that exercises this functionality? An execution test is probably a good fit for this. See the tests dir for more info and read the relevant readmes

@christiantinauer
Copy link
Contributor Author

christiantinauer commented Nov 17, 2017

I have added an execution test now.

@johnnyreilly
Copy link
Member

Your changes have broken the validateLoaderOptionNames test as you've introduced a new loader option. Please could you fix this? It should be a simple case of changing the expected 2.6 output here: https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests/validateLoaderOptionNames/expectedOutput-2.6 (both files I think)

@christiantinauer
Copy link
Contributor Author

My bad. Fixed the test.

@johnnyreilly
Copy link
Member

This makes configurations possible, where the tsconfig file is located in a npm package or elsewhere in the project structure.

I wonder if you could expand upon this a little. Specifically, I'm interested in a scenario you presently have that this change will unblock / help with. I suspect this is a good idea, but I'd like to understand the benefit of this behaviour better.

@johnnyreilly
Copy link
Member

Also, given your editor is got going to privy to the ts-loader change you might find different errors reported by ts-loader in comparison with tsc

@christiantinauer
Copy link
Contributor Author

christiantinauer commented Nov 18, 2017

This makes configurations possible, where the tsconfig file is located in a npm package or elsewhere in the project structure.

I wonder if you could expand upon this a little. Specifically, I'm interested in a scenario you presently have that this change will unblock / help with. I suspect this is a good idea, but I'd like to understand the benefit of this behaviour better.

I am currently working on a typescript version of create-react-app. The idea behind react-scripts is to have a single dependency in your app-project and get updates/fixes for webpack-configs, jest-configs and so on. You can stay focused on developing your app and not have to worry about the surroundings.

Therefore i want to "hide" the tsconfig-file in the dependencies, so I can update them with new releases or fix them without bothering the end user (except from updating the main dependency). There is already a ts-version of react-scripts. But it initially creates a tsconfig.json file in the created app and loads it from there (so no possibility to update it without interaction from end user). Also the end user is able to change the config file and might break the whole process. The problem with not having the tsconfig.json in the app directory is the "basePath" when parsing the config (this also goes on with tslint).

Long story short, I don't want the config file to be in the app directory but the base path for ts/tslint should still point to the app directory.

Also, given your editor is got going to privy to the ts-loader change you might find different errors reported by ts-loader in comparison with tsc

I solved this problem with a tsconfig.json file in the app directory (I know :)) and using the extend setting. Similiar to the suggestions of create react app for eslint with editors.

{ "extends": "./node_modules/ts-config-react-app/index" }

@johnnyreilly
Copy link
Member

Thanks for the explanation - that's super helpful. I'm sick as a dog right now and will review this properly when better. I think this will probably be merged unless something suddenly occurs to me. It seems fine as long as it's behaviour that lives behind an option.

Whilst I pop off to cough up a lung do you want to update the readme with your option? Ta. Probably worth mentioning that the behaviour may differ from tsc with this option in play.

@christiantinauer
Copy link
Contributor Author

I have added the description of the option to the README. Thanks for having a look.

Get well soon.

@johnnyreilly
Copy link
Member

Hey @christiantinauer,

I'm now feeling slightly more human and so I'm starting to look at PRs.

I solved this problem with a tsconfig.json file in the app directory (I know :)) and using the extend setting. Similiar to the suggestions of create react app for eslint with editors.

It's quite possible I'm misunderstanding you, but the purpose of this PR is so you don't need a tsconfig.json but in order to use it you need to add a tsconfig.json

I'm super confused 😀

I am currently working on a typescript version of create-react-app.

Did you know that this already exists?: https://github.com/wmonk/create-react-app-typescript ?

@christiantinauer
Copy link
Contributor Author

christiantinauer commented Nov 22, 2017

It's quite possible I'm misunderstanding you, but the purpose of this PR is so you don't need a tsconfig.json but in order to use it you need to add a tsconfig.json

I'm super confused 😀

The purpose is so you don't need it. And for the ts-loader that is true. But if you have a look at the things I added to the README, you will see that editors like VS Code will have problems when there is no tsconfig around.

Did you know that this already exists?: https://github.com/wmonk/create-react-app-typescript ?

I know and I linked it in one of the comments above.

There is already a ts-version of react-scripts. But it initially creates a tsconfig.json file in the created app and loads it from there (so no possibility to update it without interaction from end user). Also the end user is able to change the config file and might break the whole process.

Their are some differences between his and my approach:

  • I support both (js and ts) for easier transitions. (The company I work for already had a project, so I started to rework "modules" of it).
  • I use TS to Babel. (Makes upstream integration easier and I can follow the development. TS is the "typingoverlay".)
  • I want to have full control over the tsconfig.json in the future (reason for PR). (He just adds the tsconfig.json to the project initially and the end user has to update it if necessary. Also the end user can potentially break the scripts with changes to the tsconfig.json.)

@johnnyreilly
Copy link
Member

The purpose is so you don't need it. And for the ts-loader that is true. But if you have a look at the things I added to the README, you will see that editors like VS Code will have problems when there is no tsconfig around.

I have to say; I do struggle with this a little. I want to try and explain why. It feels "wrong" that with this setup that to actually do any development your first step has to be to create a tsconfig.json that extends another hidden tsconfig.json. Whilst the code will compile without that, no-one would actually be productive that way because their editor would be red and shouty until they did this. Also, if the user changed their settings in their new tsconfig.json, the new settings would apply to the editor but not to ts-loaders compilation. This creates a confusing disconnect which is not helpful.

One of the intentions of ts-loader is to, generally, be as similar in behavior to tsc as possible. We're not averse to adding options that will alter behaviour when set but the reason should be really good.

I appreciate this matters to you. This is a small change and so I'm not too worried about the impact on the codebase. However, I would ask you: do you think this is a good idea? I'm on the fence right now.

@christiantinauer
Copy link
Contributor Author

christiantinauer commented Nov 22, 2017

I totally understand your worries and that the goal of ts-loader is to follow tsc as much as possible. My personal opinion is that webpack-loaders should only handle the files they are given, which is why I like the onlyCompileBundledFiles option. This option is the reason why I want to switch from awesome-typescript-loader to ts-loader. As said this option creates a quite different behaviour compared to tsc.

Also there is absolutly no need for a tsconfig.json in your project if you don't use code editors which depends on it. As you mentioned, the workflow would work without it. Having your config files "hidden" away is a tool design decision (create-react-app) and with the extending pattern I follow their suggestion. It is a quite different approach, but I like it. With the onlyCompileBundledFiles and contextAsConfigBasePath options ts-loader could behave like for example babel-loader. This configuration follows more the original idea of webpack-loaders. Again this is my opinion.

@johnnyreilly
Copy link
Member

Sure - thanks for explaining. Let me mull this over for a bit.

@christiantinauer
Copy link
Contributor Author

Did you @johnnyreilly find some time to think about it?

@johnnyreilly
Copy link
Member

Yeah, here's where I am:

Against:

  • I don't think this is a very useful option. I don't see other use cases than your own.

For:

  • the code changes are really minimal. This is a low impact change.
  • it has a test in the execution pack and I don't see this being something that would likely be reverted by future changes
  • it would make you happy

Since the pros outweigh the cons I'm planning to merge this change. When I next get time to do a release I'll merge this and ship.

Thanks for your contribution and for bearing with me. 🐻

@christiantinauer
Copy link
Contributor Author

That is great news. Thank you for challenging my idea/contribution. Also thinking about your "Against" I came to the conclusion that this might not be only my use case, but a more general approach for fully integrated development tools like react-scripts. I guess we will see in the future.

Thank you very much. Have a nice weekend.

@johnnyreilly johnnyreilly merged commit ba72dfc into TypeStrong:master Nov 29, 2017
@johnnyreilly
Copy link
Member

Shipped with 3.2.0 🌻

@jacobdeichert
Copy link

I'll point out that I have the exact same use case as @christiantinauer. I made a cli tool (create-app) for myself and I ran into this exact same issue a few months ago but decided i'd keep the tsconfig file in my project src due to editors like vs code needing them.

With this change, i can now extend the config from inside my create-app module instead, just as @christiantinauer described. This is perfect!

@johnnyreilly thank you for merging this!

@johnnyreilly
Copy link
Member

Awesome! Thanks for sharing!

@johnnyreilly
Copy link
Member

@jakedeichert it sounds like there are issues with this option and it may be replaced with a new option. See #688 cc @christiantinauer

@jacobdeichert
Copy link

@johnnyreilly thanks for the notice! I haven't tried to take advantage of this new option yet. I'll wait until a proper fix is in place.

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