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

Implement getResolve #99

Merged
merged 2 commits into from
Sep 11, 2020
Merged

Implement getResolve #99

merged 2 commits into from
Sep 11, 2020

Conversation

qnighy
Copy link
Contributor

@qnighy qnighy commented Sep 10, 2020

Fixes #79 by implementing getResolve. getResolve is a generalization of resolve where resolver.resolve(context, request, callback) is equivalent to resolver.getResolve(null)(context, request, callback). Differences:

  • It takes options.
  • It's a curried function so getResolve itself returns a function. I guess this is meant to cache options. However options is also cached in the later stage of the resolver, so I decided not to implement caching in the thread-loader side.
  • It returns promises if callback is omitted.

Therefore message-wise we can extend the existing resolve message to allow options payload and handle them in the worker pool. In the worker pool, we delegate resolve message to getResolve if options is specified for compatibility with resolvers without getResolve (although I'm not sure there's any). Currying and promise generation are handled on the worker side.

@jsf-clabot
Copy link

jsf-clabot commented Sep 10, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

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

Good job, but we need tests for this

@qnighy
Copy link
Contributor Author

qnighy commented Sep 10, 2020

Could you provide some hints on testing? I looked at a similar PR #88 and there are no tests. At a glance, this library doesn't seem to contain any tests directly calling functions like resolve or emitWarning.

@alexander-akait
Copy link
Member

alexander-akait commented Sep 10, 2020

@qnighy You can create test with sass-loader or less-loader and with @import, they use getResolver

@qnighy
Copy link
Contributor Author

qnighy commented Sep 10, 2020

Thanks! I'll take a shot.

@qnighy
Copy link
Contributor Author

qnighy commented Sep 11, 2020

Added a test that actually runs webpack using the Node interface. I wanted to specify ./src/index.js as an entrypoint but Node rejects it saying the extension must be .mjs so I resorted to ./dist/index.js instead.

Copy link
Member

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

Good job, thanks!

@alexander-akait alexander-akait merged commit 16bbc23 into webpack-contrib:master Sep 11, 2020
@qnighy qnighy deleted the getResolve branch September 11, 2020 13:24
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.

Issue with sass-loader (this.getResolve is not a function)
3 participants