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

Graduating experimentalWatchApi from experiment to default #757

Closed
mnquintana opened this issue Apr 7, 2018 · 17 comments
Closed

Graduating experimentalWatchApi from experiment to default #757

mnquintana opened this issue Apr 7, 2018 · 17 comments
Labels

Comments

@mnquintana
Copy link

mnquintana commented Apr 7, 2018

👋 After reading up on #685 and some related issues that had cropped up over the course of testing, I wanted to ask:

  • What would it take to graduate experimentalWatchApi from an experimental API to the default watch behavior for ts-loader?
  • Are there any lingering known issues with it? (I didn't find anything from a cursory search.)
@johnnyreilly
Copy link
Member

johnnyreilly commented Apr 7, 2018

I haven't done much testing with it myself. (Surprising I know but this landed around the time that the webpack 4 port was happening and that occupied a lot of my time!)

That said, this post detailing how it's used by one of the Outlook team at Microsoft suggests that it's pretty much good to go:

https://medium.com/@kenneth_chau/speeding-up-webpack-typescript-incremental-builds-by-7x-3912ba4c1d15

I'm certainly going to be doing some more testing with it in the near future. If that pans out well then it will likely become the default watch behaviour in future.

@johnnyreilly
Copy link
Member

Just curious; do you use ts-loader at Slack?

@johnnyreilly
Copy link
Member

I'd be interested in the feedback of Vue users using the experimentalWatchApi: true option in their ts-loader options. Mostly the watch API seems to just work™️. The main question mark is over how appendSuffixTo behaves with the watch API. (This option is only used by Vue users AFAIK)

Running the tests appendSuffixTo and appendSuffixToWatch with experimentalWatchApi: true produced different output. Our comparison tests are just file diffs and a difference does not always represent a failure. So I'd be interested in a Vue user taking this for a spin and reporting back.

cc @HerringtonDarkholme @alexjoverm @prograhammer @CKGrafico @wonderful-panda @sheetalkamat @MLoughry @yyx990803 @kenotron

PS If anyone would be up for contributing a Vue execution test to our execution test pack (the execution test pack executes compiled code and so can be more informative about whether changed output is actually still valid) that would be most welcome 🌻

@beheh
Copy link

beheh commented Apr 10, 2018

We have a relatively complex project where the (initial) compile just hangs forever after enabling experimentalWatchApi.

I haven't got time to dig in right now, but can try and come up with a minimal case at some point. In the meantime, here's the webpack.config.js.

@johnnyreilly
Copy link
Member

Thanks for reporting @beheh. If you could take some time to investigate that would be tremendous. cc @sheetalkamat (aka the person who put the hard graft in to adding this functionality to ts-loader 😉 )

Just for clarification: if we do make the switch to use the watch API by default we will have an option that allows switching back to the current mechanism just in case.

Besides anything else, we still support versions of TypeScript that predate the watch API existing.

@CKGrafico
Copy link

Do we need webpack 4 for this?

@kenotron
Copy link
Contributor

We've experienced that stall as well if transpileonly is off. So that seeing isn't appropriate for production. For that we end up compile to JS and then bundle.

@johnnyreilly
Copy link
Member

johnnyreilly commented Apr 13, 2018

@CKGrafico - basically yes. It was available with 3.5 but it contained a bug which @sheetalkamat fixed.#747

@kenotron that's a shame. It would be good if there was a reproduction for that. Is that possible?

@CKGrafico
Copy link

We are not using webpack 4 yet on our projects, but I'll check when we find some time to migrate haha

@abirmingham
Copy link

Any update on this? Would love to use experimentalWatchApi but the 'experimental' part makes me skittish :)

@johnnyreilly
Copy link
Member

I'm afraid not @abirmingham. I don't have any more information than what you see in this thread. I'm hoping someone will supply a minimal repro repo so that someone can investigate a concrete issue. At present it's somewhat unknown where the dragons lie. Frustrating alas

@kiliancs
Copy link

We tried experimentalWatchApi but it caused intermittent problems with json imports:

en.js?uihost=x:100 Uncaught Error: Cannot find module '/data/x/activitygroups.json'
    at webpackMissingModule (en.js?uihost=x:100)
    at Object../i18n/en/activitygroups.json (en.js?uihost=x:100)
    at __webpack_require__ (en.js?uihost=x:20)
    at eval (index.ts:7)
    at Object../i18n/en/index.ts (en.js?uihost=x:229)
    at __webpack_require__ (en.js?uihost=x:20)
    at eval (en.init.ts?1fda:2)
    at Object../i18n/en.init.ts (en.js?uihost=x:93)
    at __webpack_require__ (en.js?uihost=x:20)
    at en.js?uihost=x:84

We haven't figured out how to reproduce this reliably. It would happen when switching branches in some cases, or simply by restarting webpack-dev-server, but only occasionally.

The problem would occur with both import x from 'some_file.json' as well as require('some_file.json')`, for what that's worth.

Upgrading webpack to 4.17.1 and removing the experimentalWatchApi gave us similar performance as we had with experimentalWatchApi and webpack 4.16.3. The problem seems to be gone.

@sheetalkamat
Copy link
Contributor

@johnnyreilly tsloader uses its own resolve module names api to resolve module name which could be causing this?

@johnnyreilly
Copy link
Member

johnnyreilly commented Aug 30, 2018

Thanks for the information @kiliancs

@johnnyreilly tsloader uses its own resolve module names api to resolve module name which could be causing this?

Thanks for that @sheetalkamat - could be worth swapping that for TypeScript's one. I've a vague idea I started doing that and then backed it out at some point but I can't remember why. I think it may be time for a reattempt.

Of course if anyone would like to submit a PR that's always welcome 😁

It's a shame that there's no reliable way to reproduce this issue! At the moment we know there be dragons but they only appear every now and then. So any fix is speculative at best 😕

@kiliancs
Copy link

False alarm @johnnyreilly. The issue still occurs without the experimentalWatchApi. It's safe to assume our problem was unrelated. Dragons indeed.

@stale
Copy link

stale bot commented Jan 19, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 19, 2019
@stale
Copy link

stale bot commented Jan 26, 2019

Closing as stale. Please reopen if you'd like to work on this further.

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

8 participants