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 regression tests for instanceof #4525

Merged
merged 3 commits into from Jun 10, 2022
Merged

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jun 10, 2022

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:
#4524

Description

To not go empty handed after the explorations in #4524, this PR adds the regression tests created there so that we have them available when we have another attempt at tree-shaking instanceof.

The core of the problem is that there are quite legitimate use cases where the only usage of a class is in the left side of an instanceof, most importantly if the left side is a function parameter and the instance is passed as a parameter to that function.

One possible approach would be to actively determine if

  • the RHS can be resolved to a class or function
  • get a reference to that one
  • find out if the left hand side references that class or references something we cannot resolve

But that would require quite a bit of new infrastructure just to support this, so I would rather hold off on this until we have a real use case.

@github-actions
Copy link

github-actions bot commented Jun 10, 2022

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#optimize-instanceof

or load it into the REPL:
https://rollupjs.org/repl/?pr=4525

@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #4525 (001639d) into master (30735b4) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 001639d differs from pull request most recent head 4547588. Consider uploading reports for the commit 4547588 to get more accurate results

@@           Coverage Diff           @@
##           master    #4525   +/-   ##
=======================================
  Coverage   98.89%   98.89%           
=======================================
  Files         208      208           
  Lines        7332     7332           
  Branches     2094     2094           
=======================================
  Hits         7251     7251           
  Misses         26       26           
  Partials       55       55           
Impacted Files Coverage Δ
src/ast/nodes/NewExpression.ts 100.00% <ø> (ø)
src/ast/nodes/BinaryExpression.ts 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 30735b4...4547588. Read the comment docs.

@lukastaegert lukastaegert merged commit 1792cdf into master Jun 10, 2022
@lukastaegert lukastaegert deleted the optimize-instanceof branch June 10, 2022 09:05
pos777 pushed a commit to pos777/rollup that referenced this pull request Jun 18, 2022
* Treeshake instanceof

* Fix side effect and self-reference handling

* Remove feature and add regression test
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.

None yet

1 participant