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 #648: Add setDefaultWaitOptions function #658

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

michens
Copy link

@michens michens commented Jul 26, 2021

What:

Fix for #648. Add setDefaultWaitOptions function

Why:

It simplifies configuration in environments where the defaults may not be optimal

How:

  • Created a function to override default time options
  • Added tests to ensure behavior mimics that of the per-call options
  • Re-exported at various levels to match existing patterns and ensure availability regardless of importing at the top level or for a specific renderer

Checklist:

  • Documentation updated
  • Tests
  • Ready to be merged
  • Added myself to contributors table

Had issues with docz. Needed to remove repo field from package.json and update glob pattern for files in doczrc.js to build updated documentation. These changes are not included as they presumably do not apply to the pipeline here

@codecov
Copy link

codecov bot commented Jul 27, 2021

Codecov Report

Merging #658 (90d0fe0) into main (a54eeb3) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 90d0fe0 differs from pull request most recent head 62fa16d. Consider uploading reports for the commit 62fa16d to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main      #658   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          235       236    +1     
  Branches        33        33           
=========================================
+ Hits           235       236    +1     
Impacted Files Coverage Δ
src/core/index.ts 100.00% <ø> (ø)
src/dom/pure.ts 100.00% <ø> (ø)
src/native/pure.ts 100.00% <ø> (ø)
src/server/pure.ts 100.00% <ø> (ø)
src/core/asyncUtils.ts 100.00% <100.00%> (ø)
src/pure.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 a54eeb3...62fa16d. Read the comment docs.

@mpeyper
Copy link
Member

mpeyper commented Jul 27, 2021

Thanks @orokanasaru for putting this together. Your comments about docz running on the PR pipeline made me wonder because I thought we had Netlify deploy preview running on All PR branches... Turns out that our Netlify config was never updated when we switched from master to main for our production branch and we haven't had any doc updates running since May! So thanks for highlighting that as well.

Unfortunately you're correct about the build issue for docz which appears to be an open issue for them. I'm reluctant to release a new feature without the accompanying documentation and I'm more reluctant to remove the repo field from the package.json to fix it, so I'm still looking for a way around this to get this in.

Honestly, this is not the first time docz has just randomly broken on us and I've been considering migrating the docs to something else for a while now. This may be the final trigger for it (we'll see how my investigation tonight into working around it goes).

I'll give the changes a review now anyway.

docs/api-reference.md Show resolved Hide resolved
src/__tests__/asyncHook.test.ts Show resolved Hide resolved
Comment on lines -13 to +19
const DEFAULT_INTERVAL = 50
const DEFAULT_TIMEOUT = 1000
let defaultInterval: number | false = 50
let defaultTimeout: number | false = 1000

function setDefaultWaitOptions({ interval, timeout }: WaitOptions) {
defaultInterval = interval ?? defaultInterval
defaultTimeout = timeout ?? defaultTimeout
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need some way of resetting the defaults back to the initial values?

I'm just thinking about someone that wants to update the defaults for a single test file so they set it in the beforeAll and want to reset it in the afterAll. Because we don't export the initial default values, they would have to just hard code the documented default values and hope we never change them.

The options I see are:

  1. export some constants for them to use with setDefaultWaitOptions
  2. export a resetDefaultWaitOptions utility

Or maybe both? Or maybe there's another option I'm not seeing?

Copy link
Member

Choose a reason for hiding this comment

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

I think exporting the constants makes more sense than a function imo

Copy link
Member

@mpeyper mpeyper Aug 2, 2021

Choose a reason for hiding this comment

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

I've been wondering about new options into renderHook? A render helper can then be extracted like normal for shared setup.

Copy link
Author

Choose a reason for hiding this comment

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

@mpeyper, can you expand on your idea? I will export initial constants otherwise

src/core/index.ts Outdated Show resolved Hide resolved
@silviuaavram
Copy link

I am also interested into a better workaround for that repo.browsetemplate.replace issue in docz.

@mpeyper
Copy link
Member

mpeyper commented Jul 29, 2021

@silviuaavram sorry, I don't have any good news. I'm currently porting our docs over to docusaurus 🤷‍♂️

@silviuaavram
Copy link

Cool :). While doing this you could probably work around as I did now, doczjs/docz#1635 (comment). Let me know if this works for you!

mpeyper and others added 2 commits July 30, 2021 21:50
Co-authored-by: Michael Peyper <mpeyper7@gmail.com>
@mpeyper
Copy link
Member

mpeyper commented Aug 31, 2021

Hi @orokanasaru, just checking in to see where you're at with this. Is it still something you want to pursue?

@michens
Copy link
Author

michens commented Sep 1, 2021

@mpeyper, you said you did not want to release until you resolved the documentation issues. If that is no longer a blocker, I can return to finish this up

@mpeyper
Copy link
Member

mpeyper commented Sep 1, 2021

@orokanasaru yeah, for sure.

What are your thoughts on the thread about resetting the values?

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

4 participants