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

Add browser globals to known globals and prevent "typeof" side-effects #3117

Merged
merged 4 commits into from Sep 16, 2019

Conversation

lukastaegert
Copy link
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Resolves #3115

Description

This will make sure that typeof globalVariable is not considered a side-effect. Note that typeof globalFunction() would still be a side-effect as it should be. Also, unfortunately Rollup is not yet capable of "remembering" the result of such a check, so in

let x;
if (typeof myGlobal !== undefined) {
  x = myGlobal; // This will still trigger a side-effect
} 

@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #3117 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3117      +/-   ##
==========================================
+ Coverage   89.28%   89.28%   +<.01%     
==========================================
  Files         165      165              
  Lines        5731     5732       +1     
  Branches     1740     1741       +1     
==========================================
+ Hits         5117     5118       +1     
  Misses        380      380              
  Partials      234      234
Impacted Files Coverage Δ
src/ast/nodes/UnaryExpression.ts 76.19% <100%> (+1.19%) ⬆️
src/ast/nodes/shared/knownGlobals.ts 100% <100%> (ø) ⬆️

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 13b27ca...0d12dbb. Read the comment docs.

@Conduitry
Copy link
Contributor

👍 This seems like a reasonable way of getting at the same end result for my case. Thank you!

// @ts-ignore
__proto__: null,
[ValueProperties]: IMPURE
};

const PURE_FUNCTION: GlobalDescription = {
// We use shortened variables to reduce file size here
Copy link
Contributor

@Conduitry Conduitry Sep 16, 2019

Choose a reason for hiding this comment

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

Nit, but maybe stick this comment above /* OBJECT */ instead?

@lukastaegert lukastaegert merged commit 86d98cb into master Sep 16, 2019
@lukastaegert lukastaegert deleted the gh-3115-extend-known-globals branch September 16, 2019 10:28
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.

Accessing unknown globals inside a typeof guard shouldn't have side-effects
2 participants