Skip to content

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