Skip to content

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

Merged
merged 1 commit into from
Oct 8, 2019
Merged

feat: update lib #54

merged 1 commit into from
Oct 8, 2019

Conversation

mihar-22
Copy link
Collaborator

@mihar-22 mihar-22 commented Oct 8, 2019

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

  • Added .lock files to gitignore as it does more harm than good when working with contributors.
  • Added a few missing fields to the package.json such as license, engines, keywords, bugs etc.
  • Made it clear in package.json that it is semantically-released.
  • Updated all deps to the latest and removed any unnecessary ones.
  • Simplified a lot of the projects configs and removed unnecessary ones.
  • Moved away from kcd-scripts because it makes it harder for contributors and maintainers to understand what is actually happening under the hood. The build tools required are really simple anyway so I added them all such as linting with eslint, building with babel, testing with jest and that's about it.
  • Added some pre-commit hooks to lint and test files + lint commit messages.
  • Added the missing Changelog.md file.
  • Updated travis to test node (8, 10, 12) and use yarn instead of npm to speed it up.
  • Cleaned up the Readme.md file and fixed anything broken.
  • Added myself as a contributor.

Code

  • Switched from jest-dom to @testing-library/jest-dom and added to Jest setup.
  • Added in automatic cleanups to be run in the jest afterEach block.
  • Added in a dont-clean-up-after-each file to avoid automatic cleanups. There's also an option to pass in the argument process.env.STL_SKIP_AUTO_CLEANUP to prevent it as well.
  • Both the features above are found in other testing-library projects as well. It saves a fair few lines of boilerplate code and time.
  • Override @testing-library/dom fireEvents method and make it run async by flushing pending state changes with tick. I know you don't have too as a simple await works but it's better to have it in code and clear, ontop it helps with adding proper types to the project.
  • Added unmount to render results as a convenience function to call component.$destroy().
  • Added rerender to render results to rerender a fresh component on the target.
  • The two changes above are mainly because the 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.
  • Moved and added a bunch more tests to ensure everything is working as intended.
  • There was missing render options such as the optional container and testing library queries. This was preventing users from passing in queries to the getQueriesForElement function. This addresses issue Issue: custom queries option is disregarded when passed to render function #52.
  • Added types (index.d.ts) to the project.

I think that's everything ... Sorry for the slightly large update, hopefully it all looks good!

@benmonro
Copy link
Member

benmonro commented Oct 8, 2019

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.

@benmonro benmonro closed this Oct 8, 2019
@kentcdodds
Copy link
Member

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

@benmonro benmonro reopened this Oct 8, 2019
@benmonro benmonro merged commit da44d97 into testing-library:master Oct 8, 2019
@benmonro
Copy link
Member

benmonro commented Oct 8, 2019

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

@benmonro
Copy link
Member

benmonro commented Oct 8, 2019

🎉 This PR is included in version 1.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@benmonro
Copy link
Member

benmonro commented Oct 8, 2019

@mihar-22 I added you as a maintainer to the project.

@mihar-22
Copy link
Collaborator Author

mihar-22 commented Oct 8, 2019

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__/**'",

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

Copy link
Collaborator Author

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 :)

Copy link
Member

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants