fix: eliminate side effect from modern utils #6953
Conversation
| allBrowsers[browser] = semver.coerce(ModernBrowsers[browser]) | ||
| return allBrowsers | ||
| }, {}) | ||
| const modernBrowsers = new Proxy(ModernBrowsers, { |
There was a problem hiding this comment.
Still, we are always initializing a Proxy. What if we instead use getModernBrowsers which in the first call evaluates the value and keep in _modernBrowsers for next calls?
There was a problem hiding this comment.
Proxy get won't be called at initialization, it's same as using _modernBrowsers
There was a problem hiding this comment.
Would also prefer using memoization here. Seems cleaner then using a Proxy
There was a problem hiding this comment.
I also like Proxy self-memo pattern :)
There was a problem hiding this comment.
The only thing that should check later is that ensure rollup doesn't count this as a side-effect and tree-shaking works for the const variable.
There was a problem hiding this comment.
@clarkdo Doesnt seem to work:
ERROR Webpack build for lambda failed [ 11:56:45
'../node_modules/@nuxt/utils-edge/dist/utils.js\n' +
"Module not found: Error: Can't resolve 'semver/functions/coerce' in 'nuxt-lambda/node_modules/@nuxt/utils-edge/dist'",
'../node_modules/@nuxt/utils-edge/dist/utils.js\n' +
"Module not found: Error: Can't resolve 'semver/functions/gte' in 'nuxt-lambda/node_modules/@nuxt/utils-edge/dist'"
]
There was a problem hiding this comment.
@clarkdo This is because I had still stubbed semver btw, let me check what happens if i dont stub it
There was a problem hiding this comment.
When I dont stub semver I get a runtime error, have to check it. But nuxt/utils load is as follows:
Total time is 8.2ms for loading/parsing @nuxt/utils-edge. The webpack require is signal-exit, the small yellow block is escapeRegExp and the other block already shows the function.
There was a problem hiding this comment.
From log, the error is from isModernBrowser, semver is required at that time for browser detection if you're using modern server mode.
|
|
||
| export const isModernBrowser = (ua) => { | ||
| if (!ua) { | ||
| return false | ||
| } | ||
| const coerce = require('semver/functions/coerce') |
There was a problem hiding this comment.
Is it ok require called per-request? (TBH no idea of perf impact when we have cache)
There was a problem hiding this comment.
This code is called in nodejs and I think require.cache is pretty fast
Codecov Report
@@ Coverage Diff @@
## dev #6953 +/- ##
=======================================
Coverage 62.72% 62.72%
=======================================
Files 82 82
Lines 3305 3305
Branches 899 899
=======================================
Hits 2073 2073
Misses 989 989
Partials 243 243
Continue to review full report at Codecov.
|
Types of changes
Description
Checklist: