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

#621 kills ts-loader dependant builds #626

Closed
loilo opened this issue Sep 7, 2017 · 15 comments
Closed

#621 kills ts-loader dependant builds #626

loilo opened this issue Sep 7, 2017 · 15 comments
Labels

Comments

@loilo
Copy link
Contributor

loilo commented Sep 7, 2017

Okay, seems that I've screwed up with #621.

TL;DR: We probably should revert #621 and release 2.3.6 as soon as possible, subsequently rethink the solution for #619.


What happened?

My assumptions about the basePath overriding the rootDir were wrong. If I'd read the TypeScript documentation more carefully, I should have noticed that rootDir's only use is "to control the output directory structure with --outDir". That means, it's not taken into effect if no outDir option is specified.

Not taking this into account, my added comparison test does not use outDir and therefore did not cover this possible point of failure:

The rootDir gets appended to the basePath if an outDir is specified. Therefore, setting the basePath to the value of the rootDir will inevitably cause the build to break.


What to do?

It's probably not too hard to think of an alternative solution, but to have ts-loader dependants breaking for as few people as possible, we probably should revert the changes from 2.3.5 and publish 2.3.6 as soon as possible. I'm aware that this now is a real breaking change but I can't think of a better solution from the top of my hat.

The current implementation will have to go away anyway since it's fatally flawed, I guess this is pretty much worst case for anything using semver.


Again, I'm really sorry about this.

@johnnyreilly
Copy link
Member

First of all: don't worry about it. These things happen; I've done it before and I'll probably do it again. What matters now is dealing with the issue so people aren't too impacted. Would you be able to submit a PR that reverts #621? I'm on a train right now and not near a computer. If you could do that then I should be able to get 2.3.6 out in pretty short order.

@loilo
Copy link
Contributor Author

loilo commented Sep 7, 2017

Will do.


FWIW, I know that those things happen, they happen to me as regularly as they probably happen to any programmer. However till this date I've only broken internal tools or, worst case, client websites. Never tools used by thousands of peoples.

My head's fine with things like that happening but my emotions can't currently deal with producing mistakes just as stupid as that. 😉

@loilo
Copy link
Contributor Author

loilo commented Sep 7, 2017

Changes reverted in #627. Only reverted my commits of course, other changes of the release haven't been touched.

Btw, my changes apparently had sneaked in a .DS_Store file. We may want to add some macOS system files to the .gitignore in the future. 🙃

@loilo
Copy link
Contributor Author

loilo commented Sep 7, 2017

Another btw: Reverting pull requests seems to be possible with built-in GitHub features. 👍

@johnnyreilly
Copy link
Member

Nice! Is that what you used to create the PR?

@loilo
Copy link
Contributor Author

loilo commented Sep 7, 2017

Nope, I googled that after submitting the pull request. I really just did two simple git revert commands.

@johnnyreilly
Copy link
Member

Fixed with v2.3.6

@johnnyreilly
Copy link
Member

Hey @loilo,

Just a heads up, I'm planning to release ts-loader v3.0.0 in a month or so. It's going to be a major version change as I'm planning to drop support for TypeScript 1.x. (No-one is using ts-loader for that AFAIK and we can simplify the codebase if we do that.)

As part of that I'll plan to drop support for configFileName.

@psurrey
Copy link

psurrey commented Sep 11, 2017

Hey @loilo ,

with version 2.3.5 we have had issues compiling our sources with webpack + ts-loader, since modules could not be resolved: error TS2307: Cannot find module...

I have seen that you have reverted that in 2.3.6. Today I checked again with the latest version (2.3.7) but the error still exists...

Our project structure is basically the following:

/project/
├── src/
│   └── main.ts
│   └── tsconfig.json
└── webpack.config.js

In webpack.config we configured ts-loader with configFile: './src/tsconfig.json',

and in tsconfig.json we only have "baseUrl": "." which worked fine until now.

Could you provide some information about what the correct tsconfig-configuration is?

Thank you!

@johnnyreilly
Copy link
Member

@psurrey thanks for reporting. Would you be able to provide a minimal repro? This might make @loilo's life easier!

@loilo
Copy link
Contributor Author

loilo commented Sep 11, 2017

Hey @psurrey, as per configuration description, the correct configuration in your case would be configFile: './tsconfig.json':

"a relative path to the configuration file. It will be resolved relative to the respective .ts entry file."

You may even just provide the file name tsconfig.json. This should find the file as well.

However, I'm not sure how your configuration could ever have worked, even in 2.3.5...

Let me know if this worked for you. :)

@johnnyreilly
Copy link
Member

Thanks for responding @loilo 👍

@loilo
Copy link
Contributor Author

loilo commented Sep 11, 2017

The motivational maintainer assistance is real. 😁

@psurrey
Copy link

psurrey commented Sep 13, 2017

Thank you guys for responding.

I tried to reproduce my issue in a new project, but wasn't able to do so. So now I'm still trying to figure out why it doesn't work in my actual project 🤔

@loilo The issue occurred with version 2.3.5. So version 2.3.4 works for me, but versions >2.3.5 don't...

@loilo
Copy link
Contributor Author

loilo commented Sep 13, 2017

Hum. I guess then some repro repo would be useful to pin this down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants