greensock / GSAP

GSAP (GreenSock Animation Platform), a JavaScript animation library for the modern web
https://gsap.com
19.56k stars 1.72k forks source link

const in output JS? #272

Closed sgehrman closed 6 years ago

sgehrman commented 6 years ago

Hey, I'm not much of a JS guy, but I've had complaints from people on old versions of Safari.

Older versions don't like const keyword.

Using webpack and const is in my build. Let me know if I did something wrong.

*!

sgehrman commented 6 years ago

maybe I need the umd. I'll try that.

sgehrman commented 6 years ago

import { TweenMax, Power2 } from 'gsap/umd'

Can't resolve 'gsap/umd'

webpack seems to think this is a relative path.

I could add node_modules/gsap to webpack so it transpiles that. But maybe you should fix the gsap package?

sgehrman commented 6 years ago

So, this seems to be a solution, but let me know if you have a better idea.

in webpack config.

{
      test: /\.js$/,
      loader: 'babel-loader',
      // exclude: /node_modules/,
      exclude: function(modulePath) {
        return /node_modules/.test(modulePath) &&
          !/node_modules\/gsap/.test(modulePath);
      }
    }

That will transpile only node_modules/gsap and not the rest of node_modules

gsap is the only package I used that includes const keyword which breaks old versions of Safari.

jackdoyle commented 6 years ago

@sgehrman I think you just forgot to define the class you were importing from, like:

import { TweenMax, Power2 } from 'gsap/umd/TweenMax';

As for the old Safari thing, that's interesting; I didn't realize Webpack would leave ES6 stuff in there like that. Hm. Yeah, it'd be pretty easy to change those to "var" instead I guess. I can do that in the next release. But again, the UMD version should be work great for you - just define the class you're importing from.

MasterJames commented 6 years ago

To embrace 'let' as much as possible, is my recommendation. It's the proper way to var, so var is just a legacy mistake, while const is equally redundant and less flexible for hot-code loading being pushed out etc. Anyway it's not absolute but what I feel instinctively is best most of the time.

jackdoyle commented 6 years ago

@MasterJames if one of the complaints is that old versions of Safari are choking on "const", I can't imagine "let" would be any different since it's also ES6. Plus I've read that let and const are both slower than "var" in some browsers. So my hunch is that it'd be best here to just switch it to "var". If anyone has objections, let me know.

MasterJames commented 6 years ago

The performance issue was a temporary issue that is no longer a concern. There is evidence of that out there. Maybe try in your personal situation and see if it makes a significant difference for you and your code on the target platform browser etc. [ P.S. 2 less chars is let or var to const, times all declarations.] Anyway performance should not be the final word. This thread I quickly dug up https://softwareengineering.stackexchange.com/questions/274342/is-there-any-reason-to-use-the-var-keyword-in-es6 which has a reference to Doug Crockford and his explanations [good videos on his personal sight I think] that is of a similar origin to my opinions on the subject. Rarely I diverge from his wisdom like don't bother with const [main reason is hot code loading I just overwrite code stored in object trees]. There's more to research on the subject just avoid anything too old on the subject as V8 optimizations are still ongoing so eventually they should be bested for the proper way to 'let'. It's like Doug says we would have fixed var but it was breaking in some poor legacy code so they were forced to keep the old badly done var and renamed the proper good corrected version 'let'.

jackdoyle commented 6 years ago

@MasterJames Are you suggesting that using let instead of const (or var) will solve the Safari issue mentioned above? Do you think the old version of Safari will recognize let but not const?

I didn't want this thread to become about theoretical benefits of let/const over var - I'm more focused on ensuring compatibility at this point since that's such a keystone in GSAP. I do appreciate the link and suggestions though.

MasterJames commented 6 years ago

I bow to your perdicament. I am off topic then agreed. Still I encourage you to pop up an upgrade warning saying upgrade or die trying. Don't encourage legacy fools to perpetuate their foolish fodder. Devs that want or need to can dynamically load an old version.

jackdoyle commented 6 years ago

I hear you, but I really don't want to force people to downgrade to an older version and miss out on feature enhancements and bug fixes just because of a let/const preference that's largely stylistic and totally inconsequential functionally (at least in this context).

If I misunderstood and you're actually trying to say that you think using "var" here will break things for people (thus it's imperative that we use let/const), please let me know.

MasterJames commented 6 years ago

Checking Usage of said browser Safari is only 3.3 percent https://www.w3schools.com/BROWSERS/default.asp and of that? oh look 0.0% https://www.w3schools.com/BROWSERS/browsers_safari.asp

Right okay to be clear, no what I've been saying is move forward. Only use 'let' don't bother with 'const'. Warn people their browsers are out of date and tell developers that request compatibilityand support for 0.0% usage browsers to dynamically load an old version of any/all libraries (in this case greensock). It won't break anything but it's not the right thing to keep using var, and 'let' is the right choice, while 'const' is more of a conditional thing to say it's not worth using. If you have really moved forward and build code that can be hot-loaded etc.

azopyros commented 6 years ago

Break from 1.20.4 to 1.20.6. I need to gsap: 1.20.4 because latter version put const, then minify do const{ then IE break.

jackdoyle commented 6 years ago

@azopyros 1.20.6 doesn't have const at all, unless you're using it as a module because the package.json "module" does point to the /esm/ directory. You should be able to either avoid using it as a module or import directly from the root or even the /src/uncompressed/ directory if you prefer. Or use 1.20.4 of course.

glebmachine commented 6 years ago

Yeah, that makes me cry, again.

"module" pointer should point to ES5 module, which only use import/export. No arrow functions / const / let / spread and other feature is allowed.

Further reading: https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features

Please, replace const with var and then build will respects specs.

glebmachine commented 6 years ago

@azopyros @MasterJames @sgehrman guys, i've forked repo to https://github.com/glebmachine/GreenSock-JS and replaced const with var.

Everything work like a charm. Made fork for yourself, or use mine. Just add to package.json:

    "gsap": "https://github.com/glebmachine/GreenSock-JS.git",
MasterJames commented 6 years ago

Nice. Thanks.

mattcolman commented 6 years ago

can we please change const to var. it's breaking our iOS 9 users on safari.

glebmachine commented 6 years ago

@jackdoyle could you please?

jackdoyle commented 6 years ago

Absolutely. 2.0.2 should be released sometime next week. Thanks for your patience :)

MasterJames commented 6 years ago

Wasn't 'let' the correct choice now. Maybe backfill it if no let found by setting let to var if it doesn't exist on some backwards incompatibility issue that should really just be forced to upgrade.

jackdoyle commented 6 years ago

I switched to "var" to avoid backward compatibility issues. It's released now in 2.0.2. "let" is part of ES6 just like "const" is, so that wouldn't be any better.

MasterJames commented 6 years ago

Okay cool. Not sure if this could be made to work somehow anyway. try {let tester = 'working';} catch () {let = var} You get the idea anyway. Agh!... let them use babel if they must and there is no way to (monkey patch to) add a let that is a pointer to the old var.

glebmachine commented 6 years ago

@MasterJames read this spec first https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features

MasterJames commented 6 years ago

Thanks for the link.