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

Should we change the value of TEST_LICENSE_KEY to be "TEST_LICENSE_KEY"? #1024

Open
aboodman opened this issue Sep 5, 2022 · 5 comments
Open
Assignees
Labels
Fixit Small fix that should go in next release given time

Comments

@aboodman
Copy link
Contributor

aboodman commented Sep 5, 2022

We have one example of a user confusing this:

https://discord.com/channels/830183651022471199/1016346958320902244

... but also making the value of the key part of the API could facilitate using it in env vars and so on.

@aboodman
Copy link
Contributor Author

aboodman commented Sep 5, 2022

@phritz for opinion.

@phritz
Copy link
Contributor

phritz commented Sep 5, 2022

SGTM. An even simpler solution might be to just update the docs to say "... pass replicache.TEST_LICENSE_KEY", but whatever you think is less easy to misuse seems fine. if we go the route of changing its value to "TEST_LICENSE_KEY" then:

  • the new code should accept both the old and the new value
  • there should probably be a brief note in the docs about the change in case they try to pass the new value manually to old code
  • we should update the sample apps to use the new way if syntactically it's any different (suspect we just change package const's value so then they wouldn't change)

@aboodman
Copy link
Contributor Author

aboodman commented Sep 5, 2022

Yeah let's change it. Low priority, but a little bit nicer for the env var use case that tslocke describes if the value seems more purposeful. We should also document the value and consider it part of the API going forward.

@aboodman
Copy link
Contributor Author

aboodman commented Sep 5, 2022

I think in JS code users will continue to prefer using the constant to avoid "magic number" code smell, but in env vars they can use the string and we will commit to maintaining it.

@arv
Copy link
Contributor

arv commented Sep 12, 2022

sgtm

@arv arv added the Fixit Small fix that should go in next release given time label Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixit Small fix that should go in next release given time
Projects
None yet
Development

No branches or pull requests

3 participants