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

Removed option "contextAsConfigBasePath" because the context given to… #688

Merged
merged 6 commits into from
Jan 20, 2018

Conversation

christiantinauer
Copy link
Contributor

… the loader is NOT the webpack.context but the directory of the current file. This makes the option useless.

As there is no possibility to get access to the webpack.context in a webpack loader, a new option which serves the same purpose is introduced.

Added option "context" (string). Allows to directly set the base path when parsing the tsconfig file. Has to be an absolute path.

… the loader is NOT the webpack.context but the directory of the current file. This makes the option useless.

As there is no possibility to get access to the webpack.context in a webpack loader, a new option which serves the same purpose is introduced.

Added option "context" (string). Allows to directly set the base path when parsing the tsconfig file. Has to be an absolute path.
@christiantinauer
Copy link
Contributor Author

christiantinauer commented Dec 4, 2017

Today I found out that webpack.context != loader.context. The loader.context always points to the directory of the currently handled file. Within a webpack loader there is no possibility to get the webpack.context path. This makes the option "contextAsConfigBasePath" quite useless. Therefore I created this PR which leads to the same behaviour with a different config option.

I am trully sorry.

@johnnyreilly
Copy link
Member

johnnyreilly commented Dec 4, 2017

Ah. Well that's a bit of a pain.

the loader is NOT the webpack.context but the directory of the current file. This makes the option useless.

How come the test that was added to the execution pack passed then?

@christiantinauer
Copy link
Contributor Author

christiantinauer commented Dec 4, 2017

Ah. Well that's a bit of a pain.

I know.

How come the test that was added to the execution pack passed then?

Well there is only one simple test and it didn't show the problems with the paths. I am currently working on a better test. I will update the PR when its done.

@johnnyreilly
Copy link
Member

johnnyreilly commented Dec 4, 2017

Thanks - I appreciate this was a mistake and you didn't intend it. If you could be extra diligent that the execution test correctly covers this new behaviour that would be helpful.

Releasing this change will be a breaking changes release which I don't like to do very often.

However, there's likely to be a breaking changes release when I merge this PR: #685

Hopefully this can come in when that does.

@christiantinauer
Copy link
Contributor Author

Reworked test "option-context".

@christiantinauer
Copy link
Contributor Author

christiantinauer commented Dec 5, 2017

@johnnyreilly I have completly reworked the test. Maybe it is a good idea to do a quick release before people start to use the option "contextAsConfigBasePath". At the moment there are no issues, connected to the option, reported.

@johnnyreilly
Copy link
Member

johnnyreilly commented Dec 5, 2017

At the moment there are now issues, connected to the option, reported.

Sorry - can you elaborate on this please?

I think I may update the README.md to mark the contextAsConfigBasePath as deprecated and suggest it is going to be replaced by the context option in the next release.

@christiantinauer
Copy link
Contributor Author

Sorry - can you elaborate on this please?

Not my day. This is a typo (I corrected the comment). I wanted to write no and not now.

@johnnyreilly
Copy link
Member

Will merge and fix tests afterwards

@johnnyreilly johnnyreilly merged commit e8a4bb0 into TypeStrong:master Jan 20, 2018
@johnnyreilly
Copy link
Member

Will look to release this with 3.3

@zackschuster
Copy link

sorry to report -- this PR breaks my build :(

my scenario:
i have a shared tsconfig referenced by a shared webpack config, both from the same npm module. unless i set contextAsConfigBasePath, i get TS18003: No inputs were found in config file 'tsconfig.json'. replacing it with the new option is possible, but i'd prefer to keep the old option if at all possible.

thanks for all your hard work on this!

@johnnyreilly
Copy link
Member

Hi @zackschuster,

I'm afraid that I'm not planning to reintroduce the old option as it didn't fulfill the requirements that @christiantinauer was aiming for. I'm sorry I broke your build; I was under the impression the old option didn't have any practical use and so removing it wouldn't be an issue. I'm sorry it has been.

Given that you can fix this by moving to the new option I'd encourage you to do so.

Once again my apologies for the misstep.

@zackschuster
Copy link

no problem! thanks for the fast reply :)

@christiantinauer
Copy link
Contributor Author

Hi @zackschuster,

context in the sense of a webpack loader is the containing directory of the currently processed file (which I didn't know, when i submitted the first PR because of a to simple test case). There is no way that I know of to get the "project context" within a webpack loader. Therefore I introduced the ts-loader option context to set it from the outside.

I guess your tsconfig.json does not contain paths like in the include/exclude/files setting. Resolving these with respect to the project context did never work.

@zackschuster
Copy link

i'm not sure how my case worked then; i did have paths in include & exclude that weren't resolved unless contextAsConfigBasePath was set.

@christiantinauer
Copy link
Contributor Author

Does your project contain subfolders?

@zackschuster
Copy link

zackschuster commented Jan 26, 2018

it's a yarn workspace, so yes: three nested frontends point at a parent node_modules, which contains the shared configuration. you can see the configuration code here.

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