Skip to content

Proposal: Adding a baseRef option #39

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 4 commits into from
Sep 18, 2019
Merged

Proposal: Adding a baseRef option #39

merged 4 commits into from
Sep 18, 2019

Conversation

chadwithuhc
Copy link
Contributor

@chadwithuhc chadwithuhc commented Jun 20, 2019

A sample proposal to add a baseRef option. This would allow routing updates to a base endpoint (e.g. users/$uid would write as users/$uid/post for a post mapper).

  • npm test succeeds
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate JSDoc comments were updated in source code (if applicable)
  • Appropriate changes to js-data.io docs have been suggested ("Suggest Edits" button)

@crobinson42
Copy link
Member

crobinson42 commented Jun 20, 2019 via email

@chadwithuhc
Copy link
Contributor Author

chadwithuhc commented Jun 20, 2019

Cool! This ticket is a PR so I added the code for you to review, but there's a few questions I had:

  1. There's no test/ folder anymore, are we adding new tests?
  2. The mocha tests won't run without the environment variables, but we aren't loading them by default. Do you mind if I add dotenv to auto-load them? I'm guessing the devs originally had it permanently stored in their environment.
  3. I can't get the docs to build again, but I believe this was an existing problem. so I can't confirm the new docs are working correctly.

If there's other things you need me to change to finish this off, let me know!

@crobinson42
Copy link
Member

Oops! I was replying from email and didn't realize it was a PR :-)

TBH - this library needs some love & attention - if you or anyone else is interested in bringing it up to date, I'll work with you to get it up to ~2019 pkg standards ;-)

I believe the ENV vars are in the circle CI environment which is also outdated and I think needs to be upgraded to v2 of circle CI compatibility. Also, I would vote for switching the tests to Jest or at the very least updating the test lib dependencies.

Also, this PR would be a feature, minor version release v3.1.0 - any objections?

@chadwithuhc
Copy link
Contributor Author

For updating the packages, do you just want latest versions or trying to switch out any of them for different libs?

CI sounds like something you'd have to handle since that will require more access. I can look into getting a base Jest testing setup with some instructions on setting up your test environment.

A 3.1.0 version is fine by me.

@crobinson42
Copy link
Member

For updating the packages, do you just want latest versions or trying to switch out any of them for different libs?

I think Jest is awesome but feel free to swap out or upgrade to whatever is easiest and stable for the project 👍

CI sounds like something you'd have to handle
I actually don't have any access to this - the original project author setup circle CI and I can't get into the configs. I can look into this a little more and see what our options are after you get the tests updated.

1 similar comment
@crobinson42
Copy link
Member

For updating the packages, do you just want latest versions or trying to switch out any of them for different libs?

I think Jest is awesome but feel free to swap out or upgrade to whatever is easiest and stable for the project 👍

CI sounds like something you'd have to handle
I actually don't have any access to this - the original project author setup circle CI and I can't get into the configs. I can look into this a little more and see what our options are after you get the tests updated.

@crobinson42
Copy link
Member

@chadwithuhc do you need some help on this still?

@chadwithuhc
Copy link
Contributor Author

So I put this off for a while cause it was confusing. I tried to get a start on it today and I think there's a few limitations preventing a switch to Jest.

If you look into the karma.start.js and mocha.start.js they are using some testing tools from the package js-data-adapter. I'm guessing these are some standard tests all adapters must pass, and we don't want to rewrite these tests. Adding Jest on top of Karma to run additional tests would be overkill and there currently are no tests specifically for this repo. The testing setup is quite confusing and I can't figure out how I would add my own tests.

At the bare minimum, I updated the testing info so people could get the tests running at least. There was info missing on setup. I also ran an npm update which only updated a couple of packages because the rest are hardcoded.

@chadwithuhc
Copy link
Contributor Author

@crobinson42 thoughts?

@crobinson42
Copy link
Member

@chadwithuhc looks good!

@crobinson42 crobinson42 merged commit 7cb7a4e into js-data:master Sep 18, 2019
@crobinson42
Copy link
Member

published v3.1.0

@chadwithuhc
Copy link
Contributor Author

awesome thanks! What do we gotta do to get the latest on npm?

@crobinson42
Copy link
Member

Oops, sorry @chadwithuhc I though I had published 3.1.0 last week. It's published now 👍🏽

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