-
Notifications
You must be signed in to change notification settings - Fork 33
feat: update lib #54
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
feat: update lib #54
Conversation
Really appreciate the PR but I would ask you to break this up into a few smaller pull requests. Also don't worry about editing the contributors stuff that is automated. But this PR is too big to merge like this. |
Personally, I think it's obvious the library needs a lot of updates/changes/improvements and @mihar-22 is doing is a great favor in making these changes. Breaking this up into smaller pull request seems like a lot of extra work to ask him to do when ultimately we'll probably merge all of this anyway |
Fair enough. Apologies. I haven't had the time to spend on Todd one. Happy to give someone else the reigns who's more into svelte these days |
🎉 This PR is included in version 1.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@mihar-22 I added you as a maintainer to the project. |
Thanks @benmonro! |
"toc": "doctoc README.md", | ||
"lint": "eslint src/**/*.js --fix", | ||
"clean": "rimraf dist", | ||
"build": "npm run clean && babel src --out-dir dist --ignore '**/__tests__/**'", |
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.
You don't need a UMD or CJS bundle for this? All the other changes seem fine (I don't know Svelte) but keeping the build/test commands running through kcd-scripts seemed like a good idea given all the other ecosystem packages use it for bundling and other commands
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.
The main export is a CJS bundle via Babel. I don't know of a use case for a UMD bundle? In the event anyone needs it we can really easily add it using microbundle.
The build process is so simple that I think it's better to be clear to other contributors and maintainers what is going on when these scripts run. It also adds an extra level of flexibility to add anything project specific to the builds and understanding the consequences of doing so. The preact-testing-library
has been good so far with the same build setup.
Of course that's just my opinion, if the testing-library uses the kcd-scripts for cross-project consistency then I can change these when I have time :)
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.
There are some use cases for UMD exports. Specifically people wanting to use these libraries in tools like puppeteer/testcafe. I think this is fine if you want to do it that way. Feel free to maintain this library as it suits you. kcd-scripts
will always be there to help if you'd like :)
I forked the lib to add a small feature but realised that dependencies were out of date, some npm errors and small things were missing. I also noticed that the lib hasn't seen a proper update in a while so I took the opportunity to give it a refresh. There's a fair few changes but I'll try my best to document everything I've done, it's still 100% backwards compatible.
Project
package.json
such as license, engines, keywords, bugs etc.Code
jest-dom
to@testing-library/jest-dom
and added to Jest setup.afterEach
block.dont-clean-up-after-each
file to avoid automatic cleanups. There's also an option to pass in the argumentprocess.env.STL_SKIP_AUTO_CLEANUP
to prevent it as well.@testing-library/dom
fireEvents
method and make it run async by flushing pending state changes withtick
. I know you don't have too as a simpleawait
works but it's better to have it in code and clear, ontop it helps with adding proper types to the project.unmount
to render results as a convenience function to callcomponent.$destroy()
.rerender
to render results to rerender a fresh component on the target.component
should not be returned in the rendering results, it defeats the purpose of testing library and goes against the guiding principle. I left it for backwards compatibility but I think it should be removed in a major update. I can bring it up as an issue later.getQueriesForElement
function. This addresses issue Issue: custom queries option is disregarded when passed to render function #52.index.d.ts
) to the project.I think that's everything ... Sorry for the slightly large update, hopefully it all looks good!