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

Use compiler.fileTimestamps cause watcher may be null #587

Merged
merged 2 commits into from
Jul 19, 2017

Conversation

zinserjan
Copy link
Contributor

fixes #585

@zinserjan zinserjan force-pushed the compiler-fileTimestamps branch from 912be01 to 85f21e3 Compare July 17, 2017 21:21
@johnnyreilly
Copy link
Member

Thanks for contributing! Do you know if this an official part of the API? I ask as we've had that happen before; see here:

https://blog.johnnyreilly.com/2017/02/under-duck-afternoon-in-open-source.html

Could you check please? @sokra may know..

@johnnyreilly
Copy link
Member

Does fileTimestamps produce the same value as getTimes? Or does it contain more / less? To what extent? I'm considering the merits of having compilerOptions in there as a fallback

@zinserjan
Copy link
Contributor Author

Does fileTimestamps produce the same value as getTimes? Or does it contain more / less?

It's atm the same, but it depends on the implementation of the watcher. The results of watcher.getTimes() are stored in fileTimestamps. See here and here.

In case of the WatchIgnorePlugin it contains more data, as WatchIgnorePlugin enhances the timestamps with all ignored files.

Performance isn't really a topic.

  • without WatchIgnorePlugin it should be faster but this in the lower ms range.
  • with WatchIgnorePlugin I would expect that it's a bit slower, cause fileTimestamps is bigger but maybe the effect of not collecting the data compensates this..

@johnnyreilly
Copy link
Member

Thanks @zinserjan - that's really helpful. I've been asking in the webpack slack channel to see if there's any reason not to go with this. No-one has advised that's the case so far. If no-one has given a compelling reason why we shouldn't take this in the next 24 hours then I reckon we're good to merge. Thanks for the contribution by the way, we really appreciate it! 🌻

@johnnyreilly
Copy link
Member

@sokra (the creator of webpack) has just confirmed this should be safe so I plan to merge. Thanks for the PR!

@zinserjan
Copy link
Contributor Author

@johnnyreilly awesome!

@johnnyreilly johnnyreilly merged commit ac17f1f into TypeStrong:master Jul 19, 2017
@zinserjan zinserjan deleted the compiler-fileTimestamps branch July 19, 2017 19:22
@johnnyreilly
Copy link
Member

Released with v2.3.1 - thanks again! https://github.com/TypeStrong/ts-loader/releases/tag/v2.3.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.

Undefined watcher in watch-run causes error...
2 participants