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

Remove IE8 plugin #992

Merged
merged 2 commits into from
Sep 9, 2017
Merged

Remove IE8 plugin #992

merged 2 commits into from
Sep 9, 2017

Conversation

zeitgeist87
Copy link
Collaborator

Remove IE8 plugin and drop IE8 support

@zeitgeist87
Copy link
Collaborator Author

I've just spent more than 3 hours, trying to get Prism to work in IE8 with the ie8 plugin. I had some minor success, but I had to make substantial changes to prism-core.js to get anything to work. I added a few extra polyfills to the ie8 plugin and rewrote some of the existing ones.

Then an unfixable issue cropped up. IE8 removes newline characters with innerHTML und textContent. Unfortunately there is no good solution for that problem. So I concluded, that IE8 support is not possible, even with polyfills and plugins. We may just as well drop it entirely.

@Golmote
Copy link
Contributor

Golmote commented Jul 3, 2016

I'd just like to say that there is a difference between deleting the IE8 plugin (that has never worked and was not officially public) and triggering errors in IE8.

I think @LeaVerou was in favor of not triggering errors in IE8, even if Prism would not work. But maybe she changed her mind?

I personally do agree we should drop support for IE8 entirely and stop caring about triggering errors on this browser, though.

@zeitgeist87
Copy link
Collaborator Author

I think @LeaVerou was in favor of not triggering errors in IE8, even if Prism would not work. But maybe she changed her mind?

Well I tested it, and we are triggering lots of errors in IE8 right now. We would have to make substantial changes to prism-core.js to change that. Or some IE8 specific code like:

if (document.documentMode < 9)
    return null;

@zeitgeist87
Copy link
Collaborator Author

I may have overstated my case in the previous post. These things cause errors in prism-core.js in IE8:

You cannot slice DOM elements. We would have to use a bit more code to get the current script:

[].slice.call(document.getElementsByTagName("script")).pop();

Object.defineProperty only works on DOM elements. We would have to use obj['__id'] instead, which makes the __id property enumerable and we have to check for it in other parts of the code:

Object.defineProperty(obj, '__id', { value: ++uniqueId });

But there are numerous other things wrong with the plugins. The file-highlight plugin causes an error because IE8 has no document.addEventListener. We would have to put an if (document.addEventListener) statement there.

@Golmote
Copy link
Contributor

Golmote commented Jul 4, 2016

You're right. And we already removed some of the IE8 checks in the past...
I'd like to hear @LeaVerou's opinion on this, but having to keep being error-proof on IE8 really is a pain in the neck. :/

@LeaVerou
Copy link
Member

Prism was released in 2012, when IE8 mattered more. In 2016, to hell with IE8. Also, indeed the plugin never worked. It was an abandoned WIP which I gave up on when I discovered the same newline issue. Feel free to remove it, as it's kind of misleading.
As for triggering errors, if there's a simple check that we can put in the beginning (I.e. If X not supported, return), fine. Anything more than that is way too much for way too little benefit.

@Golmote Golmote merged commit f7bdfd2 into PrismJS:gh-pages Sep 9, 2017
@Golmote
Copy link
Contributor

Golmote commented Sep 9, 2017

Merged!

Copy link

@svlima777 svlima777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank You!

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

Successfully merging this pull request may close these issues.

4 participants