Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

minify failure with latest versions of uglify-js, uglify-es and terser #291

Closed
kzc opened this issue Oct 4, 2018 · 8 comments
Closed

minify failure with latest versions of uglify-js, uglify-es and terser #291

kzc opened this issue Oct 4, 2018 · 8 comments

Comments

@kzc
Copy link

kzc commented Oct 4, 2018

There's a common bug in the 3.x versions of the uglify-derived minifiers (uglify-js, uglify-es and terser) that prevents javascript bundlers from working with tether@1.4.5:

ERROR: Cannot read property '_walk' of null
terser/terser#120 (comment)

NOTE: There's nothing wrong with Tether's code! This is solely an uglify/terser problem.

You can see the bug if you apply this patch and run npm install && npm run check-dist:

--- a/package.json
+++ b/package.json
@@ -13,7 +13,8 @@
   "scripts": {
     "reinstall": "del node_modules && npm install",
     "watch": "gulp watch",
-    "build": "gulp build"
+    "build": "gulp build",
+    "check-dist": "gulp build && terser dist/js/tether.js -mc && echo SUCCESS"
   },
   "repository": {
     "type": "git",
@@ -35,6 +36,7 @@
     "gulp-rename": "^1.2.2",
     "gulp-sass": "^2.0.4",
     "gulp-uglify": "^1.4.3",
-    "gulp-wrap-umd": "^0.2.1"
+    "gulp-wrap-umd": "^0.2.1",
+    "terser": "^3.9.2"
   }
 }

Would you consider applying the following patch so that the uglify-derived minifiers can avoid this error? Hopefully this would be a temporary measure until these minifiers can be fixed.

--- a/src/js/tether.js
+++ b/src/js/tether.js
@@ -766,9 +766,4 @@ class TetherClass extends Evented {
       } else {
         let offsetParentIsBody = true;
-        function isFullscreenElement(e) {
-          let d = e.ownerDocument;
-          let fe = d.fullscreenElement || d.webkitFullscreenElement || d.mozFullScreenElement || d.msFullscreenElement;
-          return fe === e;
-        }
         let currentNode = this.element.parentNode;
         while (currentNode && currentNode.nodeType === 1 && currentNode.tagName !== 'BODY' && !isFullscreenElement(currentNode)) {
@@ -807,4 +802,10 @@ class TetherClass extends Evented {
       });
     }
+
+    function isFullscreenElement(e) {
+      let d = e.ownerDocument;
+      let fe = d.fullscreenElement || d.webkitFullscreenElement || d.mozFullScreenElement || d.msFullscreenElement;
+      return fe === e;
+    }
   }
 }
@ersel
Copy link

ersel commented Oct 12, 2018

I think there is a simple workaround for this issue by simply moving out variable d one layer up. PR is here #292

@jdavidbakr
Copy link

Does uglify have an issue tracker with this for those of us who would like to follow it? For now I'm having to force tether version 1.4.4 in my projects and would like to know when I can remove that restriction.

@ersel
Copy link

ersel commented Oct 17, 2018

I think this is it:

mishoo/UglifyJS#3274

@kzc
Copy link
Author

kzc commented Nov 7, 2018

This bug was fixed in Terser.

@kzc kzc closed this as completed Nov 7, 2018
@ersel
Copy link

ersel commented Nov 7, 2018

Could you share which PR/version of terser that fixes it? @kzc

@kzc
Copy link
Author

kzc commented Nov 7, 2018

These two patches fixed it:

terser/terser#152
terser/terser#154

Everyone should be using terser@3.10.11 or a later version.

@ersel
Copy link

ersel commented Nov 7, 2018

Thanks!

@alexlamsl
Copy link

This should now be fixed in uglify-js@3.4.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants