Skip to content

Bug fix: support webpack 5 in ts-loader#1439

Merged
johnnyreilly merged 4 commits into
TypeStrong:mainfrom
einatbar:patch-1
Mar 10, 2022
Merged

Bug fix: support webpack 5 in ts-loader#1439
johnnyreilly merged 4 commits into
TypeStrong:mainfrom
einatbar:patch-1

Conversation

@einatbar
Copy link
Copy Markdown
Contributor

@einatbar einatbar commented Mar 8, 2022

Fixes Error when running ts-loader with webpack 5 : "times is not iterable"

closes #1438

Error when running ts-loader with webpack 5 : "times is not iterable"
@johnnyreilly
Copy link
Copy Markdown
Member

Thanks! Just kicked off the tests

@johnnyreilly
Copy link
Copy Markdown
Member

The build is failing at the moment?

@johnnyreilly
Copy link
Copy Markdown
Member

compiliation.fileSystemInfo._fileTimestamps.stack.forEach(times

I notice this approach depends upon _fileTimestamps - is this API for public consumption? @alexander-akait / @sokra can you advise?

@alexander-akait
Copy link
Copy Markdown
Contributor

@vankop friendly ping, I think it is regression

@vankop
Copy link
Copy Markdown

vankop commented Mar 8, 2022

hm.. maybe some watch bug.. could you create a small reproducible repo? From code this could happen (fileTimestamps=undefined). so fast fix could be just check that fileTimestamps exists (if (!fileTimestamps) return;)

@johnnyreilly
Copy link
Copy Markdown
Member

This is actually a question about types; we're getting this compilation error:

src/watch-run.ts(33,37): error TS2551: Property '_fileTimestamps' does not exist on type 'FileSystemInfo'

It suggests that there is no available _fileTimestamps property. The fact there is an _ at the start suggests it may be an internal property and intentionally not exposed with a type - is that right?

@vankop
Copy link
Copy Markdown

vankop commented Mar 8, 2022

yes, _fileTimestamps is internal property and is not presented in types.

@johnnyreilly
Copy link
Copy Markdown
Member

Okay cool. Is there a public API that can be used? Is there an fileTimestamps for instance?

@johnnyreilly
Copy link
Copy Markdown
Member

Just looking at the code and I can see there's a getDeprecatedFileTimestamps() method. But the name suggests we shouldn't rely upon it... Is there a non deprecated one by any chance?

https://github.com/webpack/webpack/blob/770dea1fb46c7c5e04016dbab1f0854037b00d81/lib/FileSystemInfo.js#L3537

@johnnyreilly
Copy link
Copy Markdown
Member

@einatbar would you like to update your pr to use getDeprecatedFileTimestamps()? Assuming that works, we could use that as a basis for a conversation with the webpack team about a non deprecated endpoint.

@alexander-akait / @vankop if you have thoughts then please do share!

@vankop
Copy link
Copy Markdown

vankop commented Mar 9, 2022

I think compiler.fileTimestamps should work.. need @sokra to take a look.

Copy link
Copy Markdown
Contributor

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use snapshot API here, there is small example how we do it in copy plugin https://github.com/webpack-contrib/copy-webpack-plugin/blob/master/src/index.js#L569

@vankop
Copy link
Copy Markdown

vankop commented Mar 9, 2022

If I understand correctly this code snippet does:

  • watch N files
  • if one file changed => regenerate definitions

So you can use 2 api methods createSnapshot (example above) and on watch callback check that snapshot is valid
But this idea seems wrong, since you will need 1 snapshot per file.. (this will decrease build performance)

So I think you can use compiler.modifiedFiles and compiler.removedFiles


and it still unclear to me why compiler. fileTimestamps does not work

@einatbar
Copy link
Copy Markdown
Contributor Author

einatbar commented Mar 9, 2022

and it still unclear to me why compiler. fileTimestamps does not work

@vankop compiler. fileTimestamps is sometimes undefined, which causes the error "times is not iterable".
I am not sure what causes it to be undefined

@johnnyreilly
Copy link
Copy Markdown
Member

and it still unclear to me why compiler.fileTimestamps does not work
@vankop compiler.fileTimestamps is sometimes undefined, which causes the error "times is not iterable".
I am not sure what causes it to be undefined

Would it be enough to guard against compiler.fileTimestamps being undefined?

@einatbar
Copy link
Copy Markdown
Contributor Author

einatbar commented Mar 9, 2022

Would it be enough to guard against compiler.fileTimestamps being undefined?

@johnnyreilly looks like it's working as well if I just guard against compiler.fileTimestamps

@johnnyreilly
Copy link
Copy Markdown
Member

Would it be enough to guard against compiler.fileTimestamps being undefined?

@johnnyreilly looks like it's working as well if I just guard against compiler.fileTimestamps

Cool - fancy updating the PR?

@einatbar
Copy link
Copy Markdown
Contributor Author

@johnnyreilly sure, on it

@johnnyreilly
Copy link
Copy Markdown
Member

Awesome, do you want to update the package.json and the CHANGELOG.md? Then I think we're ready to ship!

@johnnyreilly johnnyreilly merged commit 12f4ffe into TypeStrong:main Mar 10, 2022
@elf-mouse
Copy link
Copy Markdown

@einatbar , thank you very much :)

@einatbar
Copy link
Copy Markdown
Contributor Author

@johnnyreilly Thank you! :)

@johnnyreilly
Copy link
Copy Markdown
Member

My pleasure @einatbar !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants