extension: allow use of ES2018 features #5377
Conversation
patrickhulce
force-pushed
the
browserify_spread
branch
from
9c15792
to
d575303
Compare
patrickhulce
force-pushed
the
browserify_spread
branch
from
d575303
to
b338c9b
Compare
|
alright @paulirish @brendankenny, bring it on! |
There was a problem hiding this comment.
I was going to say can we file an issue/PR on astw, but looks like that's already been done: https://github.com/substack/astw/pull/14 :(
Interesting that @paulirish's comment isn't what we actually went with. So one last iteration on this: should we just do that and use that branch (maybe to the exact commit instead of just the branch)? It would let us specify the ecmaVersion (and even version of acorn) as an option at that point.
heh, paul just bumped us to 2.2.0 instead but seems like that goto-bus-stop's fork is really what we wanted :)
I have a ever so slight preference for not relying on a particular github user's fork and doing the hack patch ourselves, but if you and @paulirish would prefer that resolutions fix, that WFM 👍 |
| @@ -185,8 +185,7 @@ class OptimizedImages extends Gatherer { | |||
| } | |||
|
|
|||
| /** @type {LH.Artifacts.OptimizedImage} */ | |||
| // @ts-ignore TODO(bckenny): fix browserify/Object.spread. See https://github.com/GoogleChrome/lighthouse/issues/5152 | |||
There was a problem hiding this comment.
Don't forget about line 202 - 203
There was a problem hiding this comment.
nice catch thanks @underbyte! :)
lighthouse-extension/gulpfile.js
Outdated
|
|
||
| const fs = require('fs'); | ||
| // HACK: patch astw before it's required to use acorn with ES2018 | ||
| // see https://github.com/GoogleChrome/lighthouse/issues/5152 |
There was a problem hiding this comment.
can you add expand this comment some more? also note the resolution addition in package.json and the new acorn in devDeps
lighthouse-extension/gulpfile.js
Outdated
| const astwPath = require.resolve('astw/index.js'); | ||
| const astwOriginalContent = fs.readFileSync(astwPath, 'utf8'); | ||
| const astwPatchedContent = astwOriginalContent | ||
| .replace(/ecmaVersion: .* 8,/, 'ecmaVersion: 2018,') |
There was a problem hiding this comment.
straight text match since we dont plan on making this be flexible?
lighthouse-extension/gulpfile.js
Outdated
| const astwOriginalContent = fs.readFileSync(astwPath, 'utf8'); | ||
| const astwPatchedContent = astwOriginalContent | ||
| .replace(/ecmaVersion: .* 8,/, 'ecmaVersion: 2018,') | ||
| .replace(/require\('acorn'\)/, `require(${JSON.stringify(acornPath)})`); |
There was a problem hiding this comment.
string match since we dont need it global
| @@ -7,9 +7,11 @@ | |||
| "scripts": { | |||
| "watch": "gulp watch", | |||
| "build": "gulp build:production", | |||
| "debug-build": "node --inspect-brk ./node_modules/.bin/gulp build:production", | |||
There was a problem hiding this comment.
sure but imo ndb gulp build is the besttt
There was a problem hiding this comment.
maybe it is, but seeing as the only ndb I have access to is 7 years old https://www.npmjs.com/package/ndb us pleebs have to make due with this 😛
Related Issues/PRs
closes #5152