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 DOMElement plugin to format SVG in jsdom 11 #4664

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

pedrottimark
Copy link
Contributor

Summary

Fixes #4645

An SVG element has constructor name:

  • HTMLUnknownElement in jsdom 9 and jsdom 10
  • Element in jsdom 11

The regexp has always had ? but it didn’t work as intended without parentheses.

Therefore pretty-format attempted to serialize SVG elements as ordinary objects.

Test plan

Added a test (which failed first) to match Element as val.constructor.name

@SimenB Thank you for outstanding example of repro repo!

For your info, I rewrote { plugins: prettyFormat.plugins } because plugins option value needs to be an array but plugins prop value is an object :)

@SimenB
Copy link
Member

SimenB commented Oct 11, 2017

Great! This fixes both the OOM and the RangeError: Invalid string length?

@pedrottimark
Copy link
Contributor Author

Yes, I ran pretty-format-crash linked to local build. The div also contains SVG elements.

  • Is reading from a file which includes indentation only for repro or for your actual use case? Output from DOMElement plugin doesn’t look so good after it formats text nodes for white space.

  • Also for your info, something that had nothing to do with problem: when I checked the div element with W3C validator, it reported 8 occurrences of unescaped & in attributes.

@codecov-io
Copy link

Codecov Report

Merging #4664 into master will decrease coverage by 0.96%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4664      +/-   ##
==========================================
- Coverage   57.13%   56.17%   -0.97%     
==========================================
  Files         194      194              
  Lines        6565     6544      -21     
  Branches        3        3              
==========================================
- Hits         3751     3676      -75     
- Misses       2814     2867      +53     
- Partials        0        1       +1
Impacted Files Coverage Δ
packages/pretty-format/src/plugins/dom_element.js 100% <100%> (ø) ⬆️
packages/jest-editor-support/src/Snapshot.js 0% <0%> (-98.08%) ⬇️
packages/jest-environment-jsdom/src/index.js 0% <0%> (-44.45%) ⬇️
integration_tests/coverage-remapping/covered.ts 85.71% <0%> (-14.29%) ⬇️
packages/jest-snapshot/src/index.js 42.22% <0%> (-0.96%) ⬇️
packages/jest-get-type/src/index.js 96.29% <0%> (-0.26%) ⬇️
packages/jest-runtime/src/index.js 85.09% <0%> (-0.12%) ⬇️
packages/jest-util/src/fake_timers.js 90.95% <0%> (-0.1%) ⬇️
packages/jest-config/src/normalize.js 78.14% <0%> (ø) ⬆️
packages/jest-config/src/index.js 0% <0%> (ø) ⬆️
... and 6 more

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 d82925e...203f8a5. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Oct 11, 2017

Is reading from a file which includes indentation only for repro or for your actual use case?

That's production code (middle of https://www.finn.no/). I did press autoformat in intellij though, so some of the indent is from there.

it reported 8 occurrences of unescaped & in attributes.

😱 thanks for noticing!

@cpojer cpojer merged commit 9bbbfcb into jestjs:master Oct 11, 2017
@pedrottimark pedrottimark deleted the dom-element-regexp branch October 11, 2017 19:31
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pretty-format crashes with OOM
5 participants