-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Don't use deprecated querystring package #6806
Conversation
|
2d689bf
to
ebb22e6
Compare
Benchmark ResultsKitchen Sink ✅
Timings
Cold BundlesNo bundle changes detected. Cached Bundles
React HackerNews ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold Bundles
Cached BundlesNo bundle changes detected. AtlasKit Editor ✅
Timings
Cold Bundles
Cached BundlesNo bundle changes detected. Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
ebb22e6
to
4600273
Compare
a59a4ea
to
80a2b87
Compare
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.
Isn't the querystring package built into node? https://nodejs.org/api/querystring.html
let width = | ||
typeof asset.query.width === 'string' | ||
? parseInt(asset.query.width, 10) | ||
: null; |
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.
ugh that's annoying... should we even support duplicate query params?
Or maybe asset.query
should be a URLSearchParams
object?
It says:
|
…rystring # Conflicts: # packages/transformers/image/src/ImageTransformer.js
URLSearchParams
is supported in Node >=10