guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Remove Unexpected Usage of ES6 const #499

Closed jameslawson closed 7 years ago

jameslawson commented 7 years ago

Would it be possible to change all usages of const to var that are in present in browser-targeted javascript code? There are only three usages in one file (src/config.js). Commit 23a387b introduced the usage of const. Currently these three consts are breaking IE8-IE10 completely giving a syntax error.

SCRIPT1002: Syntax Error
scribe.js, line 7313 character 5

This one change would fix IE8-IE10 (assuming the necessary Polyfills are added). While IE is officially unsupported, this change is small and I think many would find it very useful:

// src/config.js
function defaults(options, defaultOptions) {
  const optionsCopy = immutable.fromJS(options);         // use var ... = ...
  const defaultsCopy = immutable.fromJS(defaultOptions); // use var ... = ...
  const mergedOptions = defaultsCopy.merge(optionsCopy); // use var ... = ...
  return mergedOptions.toJS();
}

Right now your Wiki contradicts what is actually needed to fix non-supported browsers.
From the Wiki ("Browser Support"):

Polyfills. If you want to use Scribe with non-supported browsers here is a list of polyfills you will need.

  • Object.assign

To me, the sentence: "If you want to use Scribe with non-supported browsers here is a list of polyfills you will need." is no longer true... even if you add those polyfills, scribe in IE8-IE10 will not work. You need to do both: (i) adding the polyfills and (ii) removing usages const. Then it works fine.

Scribe has been working quite well for us in IE8-IE10 (which are unsupported) w/polyfills. However we recently upgraded scribe to v3.2.0 and it no longer works because of this issue with the usages of const.

jameslawson commented 7 years ago

Pull request is here: #500

katebee commented 7 years ago

Hi @jameslawson, sorry it took so long to reply!

Thanks for raising and explaining this issue; your PR is merged 👍