-
Notifications
You must be signed in to change notification settings - Fork 787
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(screenshot): recognise clip options #5205
Conversation
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/mock-doc/serialize-node.ts | 36 |
src/dev-server/server-process.ts | 32 |
src/compiler/prerender/prerender-main.ts | 22 |
src/testing/puppeteer/puppeteer-element.ts | 22 |
src/runtime/client-hydrate.ts | 20 |
src/screenshot/connector-base.ts | 19 |
src/runtime/vdom/vdom-render.ts | 18 |
src/compiler/config/test/validate-paths.spec.ts | 16 |
src/dev-server/request-handler.ts | 15 |
src/compiler/prerender/prerender-optimize.ts | 14 |
src/compiler/sys/stencil-sys.ts | 14 |
src/compiler/transpile/transpile-module.ts | 14 |
src/sys/node/node-sys.ts | 14 |
src/compiler/prerender/prerender-queue.ts | 13 |
src/compiler/sys/in-memory-fs.ts | 13 |
src/runtime/connected-callback.ts | 13 |
src/runtime/set-value.ts | 13 |
src/compiler/output-targets/output-www.ts | 12 |
src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2345 | 367 |
TS2322 | 365 |
TS18048 | 224 |
TS18047 | 99 |
TS2722 | 37 |
TS2532 | 26 |
TS2531 | 23 |
TS2454 | 14 |
TS2352 | 12 |
TS2790 | 11 |
TS2769 | 8 |
TS2538 | 8 |
TS2344 | 6 |
TS2416 | 6 |
TS2493 | 3 |
TS18046 | 2 |
TS2684 | 1 |
TS2430 | 1 |
Unused exports report
There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
File | Line | Identifier |
---|---|---|
src/runtime/bootstrap-lazy.ts | 21 | setNonce |
src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
src/testing/testing-utils.ts | 198 | withSilentWarn |
src/utils/index.ts | 145 | CUSTOM |
src/utils/index.ts | 269 | normalize |
src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
src/compiler/app-core/app-data.ts | 25 | BUILD |
src/compiler/app-core/app-data.ts | 115 | Env |
src/compiler/app-core/app-data.ts | 117 | NAMESPACE |
src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do me a favor and update the testing instructions in the PR summary? Right now, they allude to a few steps to take care of in the new year
fixes #5208 STENCIL-1093
a36fbe9
to
c15f4c1
Compare
@rwaskiewicz I created a dev build |
@christian-bromann Sent you a DM with the action run |
@rwaskiewicz I updated the description to help verify this change. Please let me know if you can verify the resolution. I've discovered some more problems in this area, e.g. not providing a clip property at all causes issues too since Puppeteer starts headless with a 800x600px size while the default width/height properties are 600x600. So while this patch fixes the users issue, there are certainly more problems in this area. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected!
This patch has been released as a part of today's Stencil v4.11.0 release. |
What is the current behavior?
Currently the
ScreenshotOptions
passed into thecompareScreenshot
command aren't recognised as fully aswidth
andheight
are overwritten by the value stored in theemulateConfig
which is hard coded invalidateConfig
.refs #4866
fixes #5208
What is the new behavior?
Just take
width
andheight
provided by the command.Does this introduce a breaking change?
Testing
To verify this change:
git clone [email protected]:Spirielle/bug-stencil-getMismatchedPixels.git
cd bug-stencil-getMismatchedPixels
npm i
npm test
-> test passsrc/components/my-component/my-component.e2e.ts
and comment out line 13 and comment in line 16npm run test
-> test fail due toImage data size does not match width/height.
This is because the clip parameter were not recognised so the image size differs from the baseline image.
Verify fix:
npm install @stencil/[email protected]
npm test
-> test now passOther information
n/a