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

Misc stuff #749

Merged
merged 8 commits into from
Jan 5, 2022
Merged

Misc stuff #749

merged 8 commits into from
Jan 5, 2022

Conversation

jimmywarting
Copy link
Contributor

@jimmywarting jimmywarting commented Jan 4, 2022

Small misc quality of dev fixes too small for one PR

not sure if you didn't want any of this small fixes.
can easier be addressed if you just review and say what you didn't like about this PR so i can fix them.

  • i moved a require() to the top
  • and i used obj destructuring
  • and explicit path with extension

all so it matches the similarity of how esm are built & required to be (for easier transition to esm if that day ever comes)
any chain use of require('xyz').whatever... only makes it harder to refactor to esm at some point later, and esm require explicit path so...

  • used @typedef to import superagent.Response to not mix up with fetch response class...
  • and removed the use of self = this with arrow fn

Also wanted to convert TestAgent into a class, but you allow it to be called without new so changing it would be a breaking change... (unless you do something like this)

also thought about using some private class fields but don't know how far you are willing to go with modernization

@niftylettuce
Copy link
Collaborator

I think we could convert everything to classes and then encourage use of new where needed (we can do major semver bump)

@niftylettuce niftylettuce merged commit a70c1a0 into ladjs:master Jan 5, 2022
@jimmywarting jimmywarting deleted the misc branch January 5, 2022 18:41
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.

2 participants