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

Expose getPageProps as public client method #24

Closed

Conversation

gryphonmyers
Copy link
Contributor

No description provided.

@brillout
Copy link
Member

Neat. Do not hesitate to hit me with questions :-).

CONTRIBUTING.md Outdated
@@ -5,6 +5,9 @@
- [Run Test Suite](#run-test-suite)


## Environment Requirements
- Node.js 15.*.*
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fairly unrealistic, node v15 will never be LTS. Could wait for node v16, which is slated for ~4/20.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should only be a development requirement, not a deployment requirement

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, my bad, missed it was in that file, 👍

@gryphonmyers gryphonmyers changed the title Low level routing apis Expose getPageProps as public client method Apr 2, 2021
@gryphonmyers gryphonmyers marked this pull request as ready for review April 2, 2021 03:22
@gryphonmyers
Copy link
Contributor Author

@brillout I changed the name of this PR to describe a more atomic feature. This is a feature that is working the way I want it to now and I think largely isolated from any other changes I'll be proposing, so I'm marking it as ready for review.

@brillout
Copy link
Member

brillout commented Apr 2, 2021

LGTM. Do you need this published or are you fine with yarn link / npm link?

@gryphonmyers
Copy link
Contributor Author

A publish would be nice actually. Quick q on tests though - should we have unit tests covering this functionality? This feature is definitely something I could cover with a unit test. It seems like the tests in the project right now are more like end to end tests (I didn't fully get them running, but it seemed like it was installing a headless browser(?)) Could you explain a bit about how you'd like to approach testing with this project?

@brillout
Copy link
Member

brillout commented Apr 3, 2021

GitHub doesn't say so but this PR is actually merged.

There was a bug in the PR so I made some modifications. AFAICT my modifications should still achieve what you need and, while I was at it, I also implemented additional low-level interfaces:

  • import { getPageById } from 'vite-plugin-ssr/client'
  • import { getPageId } from 'vite-plugin-ssr/client/router'
  • import { getPageProps } from 'vite-plugin-ssr/client/router'
  • import { getPageByUrl } from 'vite-plugin-ssr/client/router'

Note that, contrary to your PR, getPageProps(url: string) is available at vite-plugin-ssr/client/router.

Let me know if these new functions work for you.

Yes for now there are only end-to-end tests but I've got some plans to add more granular tests in the near future.

Let's add a test for these new interfaces once we have a working implementation of a deep Vue Router integration that is documented in the README.md.

I didn't fully get them running

I improved the CONTRIBUTING.md and npm scripts.

@brillout brillout closed this Apr 3, 2021
@brillout
Copy link
Member

brillout commented Apr 3, 2021

Released in v0.1.0-beta.27.

@gryphonmyers gryphonmyers mentioned this pull request Apr 5, 2021
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

3 participants