search

instantpage / instant.page

Make your site’s pages instant in 1 minute and improve your conversion rate by 1%
https://instant.page
MIT License
6.04k stars 205 forks source link

ie11 support #31

Closed cekvenich closed 5 years ago

cekvenich commented 5 years ago

I see: type='module' and that is not supported by ie11.

Does this support ie11?

dieulot commented 5 years ago

No.

isaachinman commented 4 years ago

@dieulot Just came across this after having to do quite a bit of unnecessary detective work.

You're shipping raw, untranspiled ES6 source, without semicolons, via both npm and your CDN. This goes against all best practices in the JavaScript open source / dep world.

Are you open to adding an industry-standard build step, via Babel or Rollup? It is extremely easy with @babel/preset-env to choose sensible targets and never have to worry about things again, especially with a source code this small.

If you are, I am happy to contribute that. If you aren't, I'll be forking this repo and distributing it via a proper ES5/CJS target, for anyone that's interested.

dieulot commented 4 years ago

I’m not interested in delivering an ES5 version. The usual way to load instant.page is as a module, which handles ES6.

If you want to serve it as ES5, I assume it is for bundling. I’m not familiar with bundlers, but don’t they handle ES6 to ES5 transpilation? Otherwise, why do you want ES5?

isaachinman commented 4 years ago

At the present moment, the state of the JavaScript world is such that dependencies are not transpiled, and therefore users expect deps to be distributed to some reasonable ECMAScript target that will suit their needs.

This is a massively imperfect system, and hopefully we can move towards a transpiling-deps industry wide approach in the future, but that's where we're at now.

The assumption that you are making here is that all browsers which support prefetch also support ES6, which is not true. IE11, for example, supports prefetch but does not support modules via a script tag.

It is very, very trivial to add a transpilation step to your npm publish script. I'm not sure how you're deploying to Cloudflare, but that can be set up similarly. Respectfully, I was quite shocked to see a package with 3k+ stars is shipping ES6 without semicolons.

Again, I'm happy to contribute this work if you'll merge it.

dieulot commented 4 years ago

Thanks for your explanation. You’re the first to ask for this so there’s little demand, so I’m not interested in incorporating transpiling. Please do such a fork, I’m curious to see how that would be implemented.

I’m not sure that IE11 has support, caniuse claims Edge has support but it doesn’t. I’m not worried about not supporting IE11 as it’s a small percentage of usage and it is shrinking.

cekvenich commented 4 years ago

He would at min. be second, since this is my ticket :-). IE11 has market share ~6%, so I can't tell a business that they'll have 6% less revenue/market. So I'll use the fork, please tell me where it will be. (aside, I use typescript, since it does es5 or 6 and w/o modules/imports, just CDN). Cheers/TIA.

isaachinman commented 4 years ago

The fork is available here. You can see the change set required to add transpilation here.

You can install this es5 fork via npm here. No plans to distribute via a CDN.

@cekvenich This package immediately executes on the global scope. There's no API and no need for typing whatsoever, unless I've misunderstood you?

dieulot commented 4 years ago

Thank you.

cekvenich commented 4 years ago

Will try. Of course putting it on npm puts it on CDN such as jsdeliver.

cekvenich commented 4 years ago

@isaachinman line 91 IE 11 complains about the arrow function. Can you please use normal function please, release and let me know so I can test again.

isaachinman commented 4 years ago

@cekvenich Not sure what you're talking about. Install the dep via npm: npm i instant.page.es5. You should be consuming instantpage.es5.js. It's ES5, and there are certainly no arrow functions.

If you have further problems, you can feel free to open an issue in the fork - it's not related to this repo.

cekvenich commented 4 years ago

@isaachinman :

  1. Your repo has no issue tracking.
  2. Above I reported the line # of the error: https://github.com/isaachinman/instant.page/blob/master/instantpage.js You should check before responding.
  3. Clearly you did not test this. So after you enable issues and test ie11. let me know and I may check again. Or may not.
isaachinman commented 4 years ago

I've enabled issues.

The code is transpiled before publishing to the registry. The source itself is identical to this repo. The ES5 transpiled code is gitignored.

This is an industry standard approach.

Install via npm, or consume instantpage.es5.js via jsdeliver or whatever you want.

I suggest you employ politeness when people do you favours in the future!

isaachinman commented 4 years ago

Here's a direct link, by the way.

cekvenich commented 4 years ago

@isaachinman how are you doing me a favor?

dieulot commented 4 years ago

Note that transpiling isn’t enough, I use Element.closest which isn’t supported in IE.

Later on I plan to check to see if IntersectionObserver is implemented, and if it’s not fail silently. So a version of instant.page compatible with IE would need to diverge even more from the current source code.

isaachinman commented 4 years ago

Yes, there's a difference between core ECMAScript and certain language features. I've tested the linked version above in IE11 via BrowserStack and can confirm it works.

cekvenich commented 4 years ago

@dieulot I do polly fills to support custom elements already, including the 2 you mentioned. If there are others please let us know.