Use optional chaining to simplify the detection code

[fregante]

Mini proposal to simplify the detection code. Feel free to reject it if the drawbacks are too great.

Pros

• Readable code
• ES2020, yo

Cons

• It breaks pre-ES2020 build pipelines unless you add a babel transform for it (but I think babel/preset-env should already cover it 🤔) and transpile it down

[fregante]

Note: this code should be further refined since it doesn't yet fix:

• No-op mode doesn't work on Safari #364

[rpl]

We discussed about this proposal in our triage meeting, and given the context where the polyfill is executed it sounds reasonable to consider use the optional chaining and we are willing to proceed with this.

There a failure currently being triggered in one of the unit test, we suspect it may be the change to the first file in the diff, have you already looked into that failure?

[fregante]

have you already looked into that failure?

I assumed that my second commit would fix that. I'm not sure exactly why it would fail, but in that case this PR doesn't bring a dramatic improvement of the situation. A bugfix for #364 would have to review the whole condition anyway, so I dropped that part.

This should be ready for merge now

[codecov]

Codecov Report

Merging #377 (8ca4094) into master (2ece178) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #377   +/-   ##
=======================================
  Coverage   91.03%   91.03%           
=======================================
  Files           1        1           
  Lines         145      145           
=======================================
  Hits          132      132           
  Misses         13       13           

Impacted Files	Coverage Δ
src/browser-polyfill.js	91.03% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ece178...8ca4094. Read the comment docs.

[fregante]

Will this be merged soon? It's a 1-line change that's been open for 4 months 😅

[rpl]

Will this be merged soon? It's a 1-line change that's been open for 4 months sweat_smile

I'm sorry, for sure merging the PR takes just the press of a button, but before releasing a new version there is also some maintenance work to be done in this repo, and unfortunately I have been quite busy and haven't been able to free myself from other things enough to also do that maintenance work.

This is mainly a cosmetic change (based on the compatibility table on MDN the browser versions not providing optional chaining are not exactly "recent") and so release it wouldn't change much in practice.

I'll merge it in the meantime, and hopefully get back to this repo for the rest of maintenance work soon enough.

[davidbielik]

👎 not sure what the benefits of this change was but now i have to research and figure out how to fix a webpack issue this caused 😭

Pros

• Readable code
• ES2020, yo

Changes like this should be more thoughtful

Module parse failed: Unexpected token (25:25)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders

[fregante]

@rpl I see Babel Preset Env in the dependencies, was it not applied? I assumed code went through it to avoid mistakenly changing the output target.

[fregante]

@davidbielik As for your issue specifically, updating your modules will fix it. This package version was updated correctly, each 0.x.0 version can be semver breaking.

[davidbielik]

Thanks @fregante i'm positive it's an issue on my end, just didn't want to have to start messing with webpack and npm dependencies this for the one-line change 🤣

[PoziWorld]

Would the team consider transpiling it to ES5 for CDN-hosted versions?

I develop extensions that are also used by users of Chrome v49 on Windows XP and Firefox v56/Waterfox.

See #420.

[fregante]

for CDN-hosted versions

You can't load scripts from CDN in extensions

I develop extensions that are also used by users of Chrome v49 on Windows XP and Firefox v56/Waterfox.

You're likely using babel in your build, so you can include the polyfill in the transpilation — or keep using v0.9.0. There haven't been any real changes in this polyfill for the past 2+ years