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

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

Merged
merged 4 commits into from
Mar 10, 2022
Merged

Conversation

einatbar
Copy link
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
Member

Thanks! Just kicked off the tests

@johnnyreilly
Copy link
Member

The build is failing at the moment?

@johnnyreilly
Copy link
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
Contributor

@vankop friendly ping, I think it is regression

@vankop
Copy link

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
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

vankop commented Mar 8, 2022

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

@johnnyreilly
Copy link
Member

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

@johnnyreilly
Copy link
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
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

vankop commented Mar 9, 2022

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

Copy link
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

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
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
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
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
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
Contributor Author

@johnnyreilly sure, on it

@johnnyreilly
Copy link
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

@einatbar , thank you very much :)

@einatbar
Copy link
Contributor Author

@johnnyreilly Thank you! :)

@johnnyreilly
Copy link
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug fix: support webpack 5 in ts-loader
5 participants