Apply other loaders when updating files in watch mode #1115

iorate · 2020-05-21T02:19:31Z

Currently watch-run does not apply loaders other than ts-loader to TypeScript files when updating them in watch mode.
This behavior may lead to complication errors when files are "preprocessed" by other loaders (e.g. ifdef-loader).

iorate · 2020-05-21T05:36:59Z

I need help... 😢

One test failed in Travis CI + Node.js v10 + TypeScript v3.8.2, while all tests passed in Travis CI + Node.js v12 + TypeScript v3.8.2 and AppVeyor + Node.js v10 + TypeScript 3.8.2.

The first comparison test with transpileOnly failed:

  aliasResolution
    ✓ should have the correct output (4391ms)
    1) should work with transpileOnly
  1 passing (7s)
  1 failing
  1) aliasResolution
       should work with transpileOnly:
      Uncaught AssertionError [ERR_ASSERTION]: patch0/output.txt is different between actual and expected
      + expected - actual
           Asset     Size  Chunks             Chunk Names
       bundle.js A-NUMBER-OF KiB main  [emitted]  main
       Entrypoint main = bundle.js
      -[./app.ts] A-NUMBER-OF bytes {main} [built]
      +[./app.ts] A-NUMBER-OF bytes {main}
       [./common/components/myComponent.ts] A-NUMBER-OF bytes {main} [built]

I don't understand why [built] was output only in this environment.

johnnyreilly · 2020-05-21T05:52:00Z

Comparison tests are somewhat flaky - requeued

johnnyreilly · 2020-05-21T06:02:03Z

Thanks for the PR! We really appreciate your time. I can see this adds a dependency to https://github.com/webpack/loader-runner#readme

I don't know much about loader-runner and we tend to be cautious about adding dependencies because they don't all have great futures 😉

Can you provide some details about what loader-runner does, and some details around whether you believe it has a bright future? I ask as webpack 5 is on the horizon and I am curious as to whether loader-runner remains relevant. I have looked at the loader-runner repo but there aren't tons of docs.

You may want to reach out to @sokra and see if he can advise.

iorate · 2020-05-21T17:32:27Z

Thank you @johnnyreilly ! All tests have passed.

what loader-runner does
whether you believe it has a bright future

It applies webpack loaders to a resource and returns a result. webpack uses it internally to apply loaders (lib/NormalModule.js@4.43.0). So it is the best way to run loaders in the same manner as webpack.

I think it has a good future. webpack v5.0.0-beta.16 also depends on loader-runner (lib/NormalModule.js@5.0.0-beta.16). Changelog of webpack 5 explicitly says loader-runner was upgraded (link). We can expect it to be used by upcoming webpack 5.

johnnyreilly · 2020-05-21T17:39:03Z

src/watch-run.ts

+            .slice(loaderIndex + 1)
+            .map(l => ({ loader: l.path, options: l.options })),
+          context: {},
+          readResource: fs.readFile.bind(fs)


It looks like this will create a new instance of readFile each time updateFile runs. Would it be more performant to create this once and reuse it? In fact is the bind necessary at all?

It may be a bit efficient to cache the result of fs.readFile.bind(fs) as you say.
I think bind is necessary, because there is no guarantee that fs.readFile does not use this internally.

... In the first place, readResource is not necessary, because it is optional and defaulted to readFile. As @types/loader-runner does not mark it as optional, I pass the default value explicitly to avoid a type error.
This may be better:

runLoaders( { resource: ..., loaders: ... } as loaderRunner.RunLoaderOption,

johnnyreilly · 2020-05-21T17:43:33Z

src/watch-run.ts

@@ -27,7 +37,7 @@ export function makeWatchRun(instance: TSInstance) {
        }

        lastTimes.set(filePath, date);
-        updateFile(instance, filePath);
+        promises.push(updateFile(instance, loader, loaderIndex, filePath));


What is the significance of making this array of Promises vs just calling updateFile there and then?

runLoaders called in updateFile is an async function. We must wait for all of them to finish before calling callback.
To implement waiting for multiple async functions, I think Promise.all is a good way.

sokra · 2020-05-21T18:36:23Z

Instead of using loader-runner directly, use this.loadModule from loader context. Otherwise you are missing out webpack and other plugin modifications to the loader context.

johnnyreilly · 2020-05-21T19:11:47Z

Thanks @sokra - really appreciate you advising 🌻🥰

iorate · 2020-05-22T03:23:36Z

@sokra
Thank you for your advice.

However, this.loadModule does not seem to fit my purpose.
What I attempt is to manually get a TypeScript file preprocessed by loaders other than ts-loader.
When a rule is, for example,

{
  test: /\.ts$/,
  use: ['ts-loader', 'preprocess-ts-loader']
}

I want a result of 'preprocess-ts-loader'.
this.loadModule('!!preprocess-ts-loader!unpreprocessed-ts-file.ts', (err, source) => { ... }) seems to handle the result as a JavaScript module (actually it is a TypeScript file), to fail in most cases.

Otherwise you are missing out webpack and other plugin modifications to the loader context.

I can understand it, but I expect simply running runLoaders will work for ts-loader and the few TypeScript-preprocessing loaders in most cases.

sokra · 2020-05-22T17:38:48Z

Put a stringify loader in front to make it valid js. JSON.parse the result. This has been done before in some css related loader.

iorate · 2020-05-23T07:34:22Z

@sokra I really appreciate your kindness. The "stringify loader" approach is working well for my problem.

iorate · 2020-05-23T09:02:14Z

@johnnyreilly I replaced runLoaders with loadModule, thanks to @sokra.

(Please ignore the second commit, in which I tried raw-loader as a stringify loader)

johnnyreilly · 2020-05-23T10:30:24Z

src/watch-run.ts

+) {
+  return new Promise<void>((resolve, reject) => {
+    if (
+      loaderIndex + 1 < loader.loaders.length &&


Could you put a comment here to clarify the purpose of the 2 code branches here please?

OK, I will.

iorate · 2020-05-23T12:15:16Z

Added a comment to updateFile.

johnnyreilly · 2020-05-23T16:25:35Z

src/watch-run.ts

      }
    }

-    callback();
+    Promise.all(promises)


I can't remember if we've discussed this already, are there any issues with us moving to a Promise (ie async) approach instead of the current approach? Given we're dealing with callbacks I suspect not, but I wanted to check

I think there are no new issues introduced by the 'async' approach.

johnnyreilly

Could you update the package.json version number and add an entry to the CHANGELOG.md please?

iorate · 2020-05-24T06:15:30Z

May I "thank myself" in the changelog?

* [Apply other loaders when updating files in watch mode](https://github.com/TypeStrong/ts-loader/pull/1115) - thanks @iorate

johnnyreilly · 2020-05-24T07:33:32Z

That's the tradition!

iorate · 2020-05-24T08:09:33Z

Updated package.json and CHANGELOG.md. Thank you.

johnnyreilly · 2020-05-24T13:02:54Z

Will ship with https://github.com/TypeStrong/ts-loader/releases/tag/v7.0.5 - thanks for your help! 🌻

Apply other loaders when updating changed files

d54b294

johnnyreilly reviewed May 21, 2020

View reviewed changes

Use 'loadModule' instead of 'runLoaders'

0290e65

add a stringify loader

887bf6d

johnnyreilly reviewed May 23, 2020

View reviewed changes

Add a comment to updateFile

2616d69

johnnyreilly reviewed May 23, 2020

View reviewed changes

v7.0.5

eea2a9b

johnnyreilly merged commit d187a7c into TypeStrong:master May 24, 2020

appzuka mentioned this pull request Jul 27, 2020

Fix 2 issues with experimentalWatchApi #1159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply other loaders when updating files in watch mode #1115

Apply other loaders when updating files in watch mode #1115

iorate commented May 21, 2020

iorate commented May 21, 2020

johnnyreilly commented May 21, 2020

johnnyreilly commented May 21, 2020

iorate commented May 21, 2020

johnnyreilly May 21, 2020

iorate May 22, 2020

johnnyreilly May 21, 2020

iorate May 22, 2020

sokra commented May 21, 2020

johnnyreilly commented May 21, 2020

iorate commented May 22, 2020

sokra commented May 22, 2020

iorate commented May 23, 2020

iorate commented May 23, 2020

johnnyreilly May 23, 2020

iorate May 23, 2020

iorate commented May 23, 2020

johnnyreilly May 23, 2020

iorate May 24, 2020

johnnyreilly left a comment

iorate commented May 24, 2020

johnnyreilly commented May 24, 2020

iorate commented May 24, 2020

johnnyreilly commented May 24, 2020

Apply other loaders when updating files in watch mode #1115

Apply other loaders when updating files in watch mode #1115

Conversation

iorate commented May 21, 2020

iorate commented May 21, 2020

johnnyreilly commented May 21, 2020

johnnyreilly commented May 21, 2020

iorate commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sokra commented May 21, 2020

johnnyreilly commented May 21, 2020

iorate commented May 22, 2020

sokra commented May 22, 2020

iorate commented May 23, 2020

iorate commented May 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iorate commented May 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyreilly left a comment

Choose a reason for hiding this comment

iorate commented May 24, 2020

johnnyreilly commented May 24, 2020

iorate commented May 24, 2020

johnnyreilly commented May 24, 2020