-
Notifications
You must be signed in to change notification settings - Fork 7
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
Correctly resolve virtual resource loader path on Windows #130
Conversation
🦋 Changeset detectedLatest commit: 5204745 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/webpack/package.json
Outdated
"peerDependencies": { | ||
"webpack": "^5.27.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this decision as a comment so there's a record of it somewhere public.
Vocab has very few consumers outside of SEEK, and @vocab/webpack
's only dependent is sku
which is already using a newer version of webpack than this. So, in the interest of not needing to do any work in sku
to support a new major version, we've decided not to do a breaking change despite adding a peer dependency on webpack
(that should've already been there).
// @ts-expect-error We define our own loader context type, getOptions expects | ||
// webpack's LoaderContext type, but it's missing the target field | ||
// https://github.com/webpack/webpack/issues/16753 | ||
const config = getOptions(this) as unknown as UserConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix was only released in webpack 5.76, does this still typecheck with wepack<5.76?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works with the Webpack installed in this project. But good catch, the peer dependency should be set to ^5.37.0
webpack/webpack#13164
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we use this.target
a few lines down, that would still fail to typecheck below 5.76 right? Don't really want to set a 5.76 peer dep on this package, though we could do it with a sku update i suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just using it as a dev dep, 5.76 isn't required for consumers 🤦
Follow-up from this Slack thread where the developer was on Windows and was seeing errors. The errors were caused by the way the
virtual-resource-loader
path is resolved on Windows.The solution is to use
stringifyRequest
fromloader-utils
.I also changed all fixtures to depend on internal packages with
workspace:*
because I lost too much time debugging why my changes didn't have an effect. I pulled master and afterpnpm install
fixtures had a newer version of@vocab/*
packages than what I was developing on locally.Error log