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

fix: check if 'btoa' exists for old IE versions #479

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

okchangwon
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

If the btoa function does not exist, an error will occur as follows.

btoa is not defined

Breaking Changes

Additional Info

@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #479 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #479   +/-   ##
=======================================
  Coverage   98.68%   98.68%           
=======================================
  Files           5        5           
  Lines         228      228           
  Branches       96       96           
=======================================
  Hits          225      225           
  Misses          3        3           
Impacted Files Coverage Δ
src/runtime/injectStylesIntoStyleTag.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c623f27...4dd10b8. Read the comment docs.

@@ -187,7 +187,7 @@ function applyToTag(style, options, obj) {
style.removeAttribute('media');
}

if (sourceMap && btoa) {
if (sourceMap && typeof btoa !== 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

btoa should be undefined for old IE, can you provide screenshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

@okchangwon okchangwon Aug 14, 2020

Choose a reason for hiding this comment

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

typeof should be used to check if global variables exist.

Copy link
Member

Choose a reason for hiding this comment

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

What is IE version? It is weird, because btoa should be false here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In IE9, btoa is undefined.
Or the following code is also good.

if (sourceMap && window.btoa) {

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with typeof, weird, I can't reproduce

Copy link
Contributor Author

@okchangwon okchangwon Aug 14, 2020

Choose a reason for hiding this comment

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

I have created an example of a problematic code to reproduce the phenomenon.

<script>
if (btoa) {
  console.log("it's exist");
} else {
  console.log("it's not exist");
}
</script>

This code causes an error in IE9.

Please check the console window on this page.
http://okchangwon.xyz/naver/ie9-btoa

image

Copy link
Member

Choose a reason for hiding this comment

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

error from ie9-btoa package, not from style-loader

@okchangwon
Copy link
Contributor Author

okchangwon commented Aug 16, 2020

Let me summarize it overall.

In the style loader's applyToTag method
If obj.sourceMap option and btoa are present
SourceMappingURL is being added to CSS.

The reason for checking the existence of btoa is
This is because there is no btoa function globally in Old IE(9).

Very good so far.

But the problem is,
The way to check for the existence of btoa is wrong.

It is currently as follows.

 if (sourceMap && btoa)

If you access a nonexistent global variable (or function) like this
An error occurs regardless of browser type and version.

  • IE11

When accessing a variable that does not exist
image

  • chrome

When accessing a variable that does not exist
image

Because there is no btoa in 'IE9', an error occurs.
Even if 'btoa' exists in a specific version of'IE9',
It's a good idea to check it the right way in case it doesn't exist.

So I recommend the following method.

 if (sourceMap && window.btoa)
// or
 if (sourceMap && typeof btoa !=='undefined')

Please let me know if I'm misunderstanding the situation.
Thank you.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's merge

@alexander-akait alexander-akait merged commit 732ef8b into webpack-contrib:master Aug 17, 2020
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.

2 participants