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

Replace loader-utils with built-in webpack 5 functionality #831

Merged
merged 2 commits into from
Jul 29, 2021

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Jun 2, 2021

Webpack has built-in types now, which seem to conflict with @types/loader-utils. I think we can just remove this dependency - see webpack/loader-utils#179.

Fixes #819

@mydea
Copy link
Contributor Author

mydea commented Jun 10, 2021

I rebased on latest master.

@rwjblue
Copy link
Collaborator

rwjblue commented Jun 10, 2021

Kicked off CI

@mydea mydea force-pushed the fn/remove-types-loader branch 2 times, most recently from e45c04c to 48f47f7 Compare July 5, 2021 09:54
@chancancode
Copy link
Contributor

chancancode commented Jul 18, 2021

We can't just remove the types. We currently use getOptions here. Removing the types package would cause the import itself to become a type error/any. Instead, I believe the recommendation was that we should remove the loader-utils package itself and switch from getOptions to using the (supposedly, I haven't verified) equivalent provided by webpack 5.

@mydea would you like to work on that?

@chancancode
Copy link
Contributor

I think you can also address this while you are there. It seems like recent versions of webpack does have a type for LoaderContext, further, it takes a generic slot for the options so it could just be function hbsLoader(this: LoaderContext<HbsLoaderConfig>, ...) and this.getOptions() should Just Work™.

@mydea
Copy link
Contributor Author

mydea commented Jul 19, 2021

I will give it a try!

@mydea
Copy link
Contributor Author

mydea commented Jul 19, 2021

Hmm, does hbs-loader need to remain compatible with webpack 4? Meaning, do I need to build this in a way that it either uses the new webpack loader, or loader-utils, or can I update that package to webpack 5 and just use that?

I would go with the latter (@embroider/webpack relies on 5.x already anyhow), but if it should be the former I'm not sure what approach I should take to achieve that.

@mydea mydea changed the title Remove @types/loader-utils dependency Replace loader-utils with built-in webpack 5 functionality Jul 19, 2021
@mydea
Copy link
Contributor Author

mydea commented Jul 19, 2021

I updated the PR to update webpack peerDependency to 5 for hbs-loader, and replace loader-utils with the built-in functionality & the new types.

@ef4 ef4 merged commit 44d0381 into embroider-build:master Jul 29, 2021
@ef4
Copy link
Contributor

ef4 commented Jul 29, 2021

Thanks!

@ef4 ef4 added bug Something isn't working internal and removed internal labels Jul 29, 2021
mydea added a commit to mydea/embroider that referenced this pull request Jul 30, 2021
This was changed in embroider-build#831, but in the meanwhile also added here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@types/loader-utils results in type issues
4 participants