dougludlow / plugin-sass

SystemJS SASS loader plugin
MIT License
52 stars 27 forks source link

Replace .endsWith call with lodash variant for compatibility #42

Closed ALRO closed 8 years ago

ALRO commented 8 years ago

String.prototype.endsWith is an ES6 method which isn't implemented everywhere yet - specifically there is not a single IE which has it implemented which makes developing for IE hard when not using a polyfill.

This PR replaces the method calls with lodash function calls.

screendriver commented 8 years ago

Thank you for the pull request. But some things to note:

  1. sass-builder.js will not run in your browser. It is only for bundling at your shell (Bash, zsh and so on). There is no need to change anything because it uses Babel to run the code.
  2. sass-inject.js is meant for development time. So this code only runs when you are developing your app locally on your dev machine. When you deploy your code you should bundle it.
  3. because SystemJS and this plugin is based on Babel you don't need a polyfill for that. Everything is there out of the box. It could be that you have to do a import 'babel/polyfill'; at the very first line in your app so all ES2015 features are available. But after that you don't have to worry about any ES2015 methods that are not available in any browser. You should generally import the babel polyfill in all your apps because after that you can use all ES2015 features and don't have to worry about anything.
  4. sass-inject uses a lot from ES2015 like arrow functions, modules (import / export) and so on. For these features to work you have to use Babel. The same applies to endsWith.
  5. I testet 'foo'.endsWith in Edge, IE11, IE10 and IE9. IE11 and Edge does support endsWith :wink:

So I can merge your pull request if you really want, but it's not necessary in my opinion.

ALRO commented 8 years ago

Thank you for the quick reply and severeal explanations on how the plugin is constructed.

I am also getting an error in IE11 (using browserstack). I suppose there might be different versions of IE11 with different compatibility levels... shudder

Now, I am no expert on the usage of jspm/systemjs, but having babel/polyfill imported doesn't solve the issue for me. I've tried several things and the only thing that worked for me was adding the babel polyfill as a separate script tag before the call of the entry point of my app - which was not a solution I liked.

I tried your solution here but it doesn't seem to work for me (it errors in IE11 and lower). I assume that plugin-sass gets called before the babel polyfill is loaded...?

If there is a way to get this to work in development without having to install babel-polyfill seperately and having to add a separate script tag, I would be completely fine with that.

Cheers!

screendriver commented 8 years ago

was adding the babel polyfill as a separate script tag before the call of the entry point of my app

That's not necessary. You just have to make sure that babel/polyfill'; is loaded first in the SystemJS loader order.

I've made a pull request in your sample project that fixes your issue https://github.com/ALRO/plugin-sass-error/pull/1 :sunglasses:

ALRO commented 8 years ago

I didn't think of importing it that way. This works for me. Thanks a bunch for your help! :+1:

If I may ask: If I wanted to also add this to my production bundle to make sure they both behave the same, how would I best do this? I am using a self-executing bundle.

screendriver commented 8 years ago

I don't understand the question. If you bundle your project you set the main entry file. From there it goes trough all your imports, transpiles them to ES5 and produces one large JavaScript file. After that you include that in your index.html. Thus it should work out of the box without changing anything.