-
Notifications
You must be signed in to change notification settings - Fork 200
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
Feature: Add runtimeHooksPath
options with onBeforeWriteToDiscPath
in it
#337
Feature: Add runtimeHooksPath
options with onBeforeWriteToDiscPath
in it
#337
Conversation
@code-forger what do you think about this new feature? |
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.
Sorry for the long wait, this fell of the radar. Small comment but not going to block this.
src/diff-snapshot.js
Outdated
} = options; | ||
|
||
const writeFileSync = (pathToFile, content) => { |
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.
Stylistically i'd prefer moving this function out of diffImageToSnapshot
and renaming to something like writeFileWithHooks
and have it take runtimeHooksPath
as a third argument. that way it more clearly differentiates the custom writeFileSync
from fs.writeFileSync
as an example:
const writeFileSync = (pathToFile, content) => { | |
const writeFileWithHooks = (pathToFile, content, runtimeHooksPath) => { |
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.
I won't have time to do this modification right now, but I'll do it in the next few days!
Thanks for the review 🙇♂️
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.
FYI: I'd need 5 arguments as testPath
& currentTestName
are also used inside of this writeFileSync
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.
This is now updated
@JAdshead you can re-review if you want 👌
FYI, due to bade294, the command |
3817d04
to
15a46c7
Compare
# [6.3.0](v6.2.0...v6.3.0) (2023-11-28) ### Features * Add `runtimeHooksPath` options ([#337](#337)) ([57741a2](57741a2))
🎉 This PR is included in version 6.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Add a new top level option
runtimeHooksPath
that contains a potential path to a JS file.This file can contain multiple hooks when
diffSnapshot
is called, likeonBeforeWriteToDiscPath
(to be able to inject EXIF data to PNGs without building this feature into this library).The reason why I exposed that via a path to a JS file instead of exposing a function, is because
diffSnapshot
is sometimes called from a worker, and we cannot serialize properly functions.For now there is only
onBeforeWriteToDiscPath
but I made that generic enough to be able to add more features to it in the future.Motivation and Context
Fixes: #332
How Has This Been Tested?
I added unit tests for this feature
Types of Changes
Checklist:
What is the Impact to Developers Using Jest-Image-Snapshot?
It shouldn't impact them at all as long as they don't use this new feature