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

feat: Export TypeScript types

Merged
merged 2 commits into from Oct 12, 2019
Merged

Conversation

hedgepigdaniel
Copy link
Contributor

@hedgepigdaniel hedgepigdaniel commented Oct 1, 2019

This is so that users of this package can have information about the types exported.

The code in this package is not typechecked (the types are separate from the source code), but this could be done as a non breaking change later.

Notable choices:

  • the type of the context arguments is separate from the location type, and the first and second context have separate types
  • The location type and the ontext types default to the browser Location type
  • The location type must extend { hash?: string; action?: 'PUSH' | string }. This is the same as extending any object except that if there is a hash or action key they must be strings, and the 'PUSH' is jsut a hint to the user as to which string will have an effect.

@taion
Copy link
Owner

taion commented Oct 11, 2019

@hedgepigdaniel Sorry for the delay here. I pushed a commit improving the type defs a bit. Let me know what you think.

taion
taion approved these changes Oct 11, 2019
@taion
Copy link
Owner

taion commented Oct 11, 2019

Also updated the tooling, added Prettier, and added basic linting and testing around the types.

@taion
Copy link
Owner

taion commented Oct 11, 2019

btw let me know if you want me to cut a release with just this, or if you want us to also resolve #293 before cutting

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Oct 12, 2019

Thanks!

For my use case I probably wouldn't use the release until both issues were resolved, but it wouldn't hurt to cut one.

Re your changes, I noticed these things when updating:

  • the types of the prevContext and context arguments are the same. In my case the context is a request object (which has preferences to the previous location). So the only possible use case for the second context arg is to pass some separate extra information, which is not possible if the types have to be the same. That said, I didn't feel the need to use it, and if I do I can instead just include that extra information in my request object.
  • The action key is required in the location type. This is fine (I can set it to some string that isn't "PUSH"), although I don't see the point of having to set it to a string given that I don't want to suppress the saved position for a location that already has a saved value
  • The context argument is required for updateScroll. This makes sense I think.

So basically yes, this works well.

@taion
Copy link
Owner

taion commented Oct 12, 2019

for the action thing... that's really just push v pop. since we want to do different things for push v pop transitions.

for updateScroll and friends – the idea is that in my current use cases, the context only describes the current location (indeed, in the examples here, it's just the location). i suppose if your context already includes information on the previous location, then you can just always pass in null for prevContext. this is allowed in all cases.

@taion taion changed the title feat: export typescript types feat: Export TypeScript types Oct 12, 2019
@taion taion merged commit ed4b5bc into taion:master Oct 12, 2019
3 checks passed
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

2 participants