Replace String.trimRight() with regex in order to support IE10+ #7231

Merged
merged 3 commits into from Aug 5, 2014

Projects

None yet

9 participants

@humphd
Contributor
humphd commented Mar 18, 2014

For work I've been doing on brackets-in-browser (see https://blog.webmaker.org/webmaker-experiments-with-brackets) I had to fix this use of trimRight so it would work in IE10+. With this change, I'm able to run things in IE10+:

screen shot 2014-03-18 at 9 50 30 am

@zaggino
Member
zaggino commented Mar 18, 2014

I'd rather see

if (!String.prototype.trimRight) {
    String.prototype.trimRight = function () { return this.replace(/\s*$/, ""); }
}

somewhere in the code (or a special include for browsers) than this.

@redmunds
Contributor

No need to replace 0 spaces, so this should be:

if (!String.prototype.trimRight) {
    String.prototype.trimRight = function () { return this.replace(/\s+$/, ""); }
}
if (!String.prototype.trimLeft) {
    String.prototype.trimLeft = function () { return this.replace(/^\s+/, ""); }
}

Might as well also polyfill trimLeft.

@humphd
Contributor
humphd commented Mar 18, 2014

Sure, any suggestions as to where you want this to live? Given how early UrlParams is used, it's going to need to happen early. Do you want me to add a shim section to the require.config in src/main.js, make src/utils/shims.js' that gets loaded early insrc/brackets.js, switch to using$.trim()` instead, ???. Let me know.

@zaggino
Member
zaggino commented Mar 18, 2014

I agree you should load it very early and I guess it could be placed here
https://github.com/adobe/brackets/blob/master/src/index.html#L47-52
so it get's built into the .min file, but I'm not sure if this qualifies as thirdparty

@TomMalbran
Contributor

We should be able to just add a require before this line https://github.com/adobe/brackets/blob/master/src/brackets.js#L142 or add at to any module and if it is not loaded before that line, add a require to load it.

@humphd
Contributor
humphd commented Mar 19, 2014

I've added a new commit with one possible approach.

@peterflynn peterflynn and 2 others commented on an outdated diff Mar 19, 2014
src/brackets.js
@@ -37,7 +37,10 @@
*/
define(function (require, exports, module) {
"use strict";
-
+
+ // Load compatibility shims--these need to load early, be careful moving this
+ require("utils/Compatibility");
@peterflynn
peterflynn Mar 19, 2014 Member

Does Require make any guarantees about the order that "sibling" require() calls are loaded in? @dangoor do you have any idea?

If it's not guaranteed, I wonder if it's better to put this directly in src/main.js (cluttery as that may be) or even load it directly from a separate <script> tag before we boot up Require?

@peterflynn
peterflynn Mar 19, 2014 Member

The accepted answer there boils down to 'explicitly require Compatibility.js in every module that calls trimRight(),' which seems clumsy and more than a little fragile.

The second answer listed there -- basically, chaining async require() calls in main.js -- sounds promising though.

However, if there's any sort of guarantee that the code in this PR will work as-is, that's definitely preferable. I couldn't find any docs that said either way. Anyone have a reference or spec for RequireJS's loading order?

@peterflynn
peterflynn Mar 19, 2014 Member

I read that earlier, but it's actually very vague. It seems to only pertain to JS files that aren't wrapped in a define().

Normally RequireJS loads and evaluates scripts in an undetermined order

but then...

The order plugin ... is not needed for scripts that use define() to define modules

@peterflynn
peterflynn Mar 19, 2014 Member

Maybe we should go straight to the source :-) @jrburke, does RequireJS make any guarantee about the order that sibling require() calls are resolved in? We couldn't find anything definitive in the docs. Thanks!

More detail, if needed... if module "main" does this:

define(function(require, exports, module) {
    require("A");
    require("B");
});

Is the code in the body of A's define() guaranteed to run before B's (assuming A doesn't require B internally)? I.e., are the dependencies resolved as a sort of depth-first traversal, or is the order more nondeterministic?

@jrburke
jrburke Mar 20, 2014

@peterflynn the order is not guaranteed, for two reasons:

  • other modules could have asked for 'B' before 'main' is loaded or has its dependencies registered.
  • module factories are called once they are loaded (if all their dependencies are involved), and that load order is dependent on network load order, which is indeterminate. If 'A' needs 'C', but 'B' has no dependencies, it is likely 'B' will be defined before 'A'.
@humphd
Contributor
humphd commented Mar 20, 2014

So how about if I do something like the following in src/main.js?

define(function (require, exports, module) {
    "use strict";

    // Load the brackets module. This is a self-running module that loads and runs the entire application.     
    require(['utils/Compatibility'], function() {
        require("brackets");
    });

});
@jrburke
jrburke commented Mar 20, 2014

@humphd That would lead to 'brackets' executing before 'utils/Compatibility', since that commonjs sugar style effectively "hoists" require(StringLiteral) calls as far as dependency resolution and execution. This should work:

define(function(require){
  require(['utils/Compatibility'], function() {
    require(['brackets']);
  });
});
@peterflynn
Member

@jrburke Thanks much for the clarifications & suggestions!

@humphd I think your suggestion for main.js (corrected per @jrburke's comment) would be fine -- want to go ahead & do that here?

@humphd
Contributor
humphd commented Mar 21, 2014

Done. Thanks to James for the help.

@njx njx added medium priority and removed low priority labels May 14, 2014
@njx
Member
njx commented May 14, 2014

Bumping to medium priority since this has been languishing for awhile and the Mozilla folks are serious about in-browser :)

@njx
Member
njx commented Jun 13, 2014

Added to the Kanban board to raise visibility. https://trello.com/c/2f4gK9fi

@njx
Member
njx commented Jun 17, 2014

Per @peterflynn, sounds like the main work here is to verify that this works with our minified build.

@peterflynn peterflynn removed their assignment Jun 17, 2014
@peterflynn
Member

@njx Yep, as discussed this morning.

@humphd So sorry I haven't had the bandwidth to do that last bit of verification here. Unassigning myself so someone else on the core team can wrap this up & land it!

@dangoor dangoor added the Ready label Jun 26, 2014
@redmunds redmunds self-assigned this Jul 25, 2014
@redmunds redmunds added Development and removed Ready labels Jul 25, 2014
@redmunds
Contributor

@humphd This fix works with cloned version of Brackets, but it doesn't work when code is minified with uglify, which we do for our installer builds.

@jrburke Any suggestions?

@redmunds redmunds commented on the diff Jul 28, 2014
src/brackets.js
@@ -37,7 +37,7 @@
*/
define(function (require, exports, module) {
"use strict";
-
+
@redmunds
redmunds Jul 28, 2014 Contributor

Only changes to brackets.js are whitespace changes, so these changes should be removed.

@redmunds
Contributor

@humphd @jrburke I tried to implement this as a "shim" (Thanks @jasonsanjose for the tip) in pull request #8593, but this also doesn't work in minified build. Brackets works (based on very minimal usage), but the polyfill is not applied.

cc @peterflynn @njx

@jrburke
jrburke commented Jul 30, 2014

I do not recommend shim config. That is only useful for scripts that do not call the module APIs.

How does it fail after the build step? My guess is that you probably want to manually do an `include ["utils/Compatibility", "brackets"] for the build config (if using r.js optimizer), since the require([]) form of dependencies is skipped in dependency scanning (which is the point, they are dynamic requires, that may not need to happen)?

Although worse case in that case is that it should just lead to some dynamic loads, a bit slower, but I would not expect it to error. So knowing more about the failure might be good.

Another option is to do the compatibility work above the define() call in that main module, and not as a module, just an IIFE, so that the fixups are done before any module resolution. That may not be practical if you expect to need lots of compatibility fixes, but if it is just those two things, then the main module could just go back to its old form if those trim function work is done just above the main module define().

If that approach does not work, perhaps you are including some helper scripts that do not call define() but use trim functions? In that case, I could see that when those are inlined in a build, they will be above the definitions for those trim fixes, and since not in a define() function, will execute immediately and fail.

If that is the case, then two possibilities: In the build config, use wrap.start to inject the polyfills at the top of the build layer (without a define() wrapper, just as an IIFE). Using include: could work too, but it depends on what non-modular script is using trim functions and how it ends up in the build.

If you want to work through it in more realtime, you can ping me in IRC, #requirejs on freenode, mention it is about this bug, and I should respond.

@redmunds
Contributor

@jrburke Thanks. Very help info.

I wasn't sure how to "manually do an 'include ["utils/Compatibility", "brackets"]' for the build config", so I just removed the [] from the require statement for brackets module so it doesn't get "skipped". Also added utils/Compatibility module to list of files to be copied to dist folder (i.e. installer build).

I have a working version in PR #8595 which is built on top of this branch. The diff is in commit 873e768

@peterflynn @njx @humphd @jasonsanjose Does this address all concerns?

@jrburke
jrburke commented Jul 30, 2014

@redmunds that is probably not what you want. By using synchronous require('brackets'), in order for that to work in a network environment, the module loader will see that dependency, make sure it is executed/defined before running the main module's factory function. So it will likely already run before the utils/Compatibility module's factory function is run.

The include build option normally goes in the requirejs optimizer config, I would guess around here in the gruntfile. However, you normally want to set it where the name config option is set, but a comment in that file says that is set by usemin, and I am unfamiliar with that tooling.

@redmunds
Contributor

@jrburke Thanks again! Your guess about the gruntfile seems to be right on the mark. I have this working in PR #8595 .

@jrburke
jrburke commented Jul 30, 2014

@redmunds, you might also want to add 'utils/Compatibility' before 'brackets' in that include array, so that it is included in the bundle instead of dynamically loaded (unless that utils/Compatibility gets in the bundle some other way).

@redmunds
Contributor

@jrburke I forgot about that from your original comment. Fixed.

@dangoor dangoor merged commit b8b8ef1 into adobe:master Aug 5, 2014

1 check passed

default The Travis CI build passed
Details
@redmunds
Contributor
redmunds commented Aug 5, 2014

@humphd Merged. Thanks for your effort on the in-browser version of Brackets.

@jrburke Thanks for your guidance.

@humphd
Contributor
humphd commented Aug 5, 2014

Thanks to all of you for helping taking it over the line! I hope to bring you more like this soon--we're making good progress on in-browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment