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

Does not respect defaults set in config.exs #6

Closed
codehugger opened this issue May 27, 2020 · 9 comments
Closed

Does not respect defaults set in config.exs #6

codehugger opened this issue May 27, 2020 · 9 comments

Comments

@codehugger
Copy link

It seems that the Nanoid generate functions are not picking up the values set in config.exs.

I currently have this is my config/config.exs inside a phoenix project

config :nanoid,
  size: 21,
  alphabet: "0123456789abcdef"

When I open the shell iex -S mix I get the following

iex(1)> Application.get_env(:nanoid, :size)
21
iex(2)> Application.get_env(:nanoid, :alphabet)
"0123456789abcdef"
iex(3)> Nanoid.Secure.generate()
"yXR2LbmKTzohOefJ5ocCh"
iex(4)> Nanoid.NonSecure.generate()
"obxlccY4icVuAQO2l5vy_"

This shows that the correct configuration is loaded into the iex but it is not being picked up correctly by the module.

@codehugger
Copy link
Author

This seems to be an issue with when the config is read inside a phoenix project. I did some digging and tried recompiling the project by deleting the _build directory. When I ran IEx again Nanoid picked up the updated config.exs.

I updated the config/config.exs file again now with the following settings.

config :nanoid,
  size: 4,
  alphabet: "0123456789abcdef"

But it still uses the old config when I run IEx

iex(1)> Nanoid.generate()
"efbf07642ca92851159ed"

So again I deleted the compiled files, now only the _build/dev/lib/nanoid folder.

Now when I run IEx it has picked up the new config.

iex(1)> Nanoid.generate()
"ad88"

This is a pretty acceptable workaround since this only seems to be an issue in Phoenix projects. I won't be changing the defaults for Nanoid any time soon in the project but still a little bit annoying having to figure this out.

@railsmechanic
Copy link
Owner

railsmechanic commented Jun 15, 2020

Many thanks for your testing and your workaround. I think the problem is the use of module attributes, which were assigned during compile time. It seems that the configuration in the (dev|test|prod).exs file(s) of phoenix is not available when nanoid gets compiled and therefore it uses the fallback value which is defined using Application.get_env/3.

To avoid this issue I'll introduce a Nanoid.Configuration module which also supports runtime configuration.

@railsmechanic
Copy link
Owner

@codehugger
I've added the Configuration module to master. Could you please test it with your application? When everything works, I'll release v2.0.3.

Many thanks so far.

@codehugger
Copy link
Author

@railsmechanic I have tested this and I can confirm that a simple IEx restart is enough to pick up the new config :)

@florianb
Copy link

Does this have any impact on the performance?

I made the former config changes (for the test-env) working by calling

MIX_ENV=test mix deps.compile nanoid --force

@codehugger
Copy link
Author

codehugger commented Jun 16, 2020 via email

@railsmechanic
Copy link
Owner

railsmechanic commented Jun 16, 2020

Does this have any impact on the performance?

Name                    ips        average  deviation         median         99th %
nanoid_master        5.40 K      185.23 μs    ±58.53%      157.40 μs      468.39 μs
nanoid               6.29 K      159.06 μs    ±61.38%      135.91 μs      445.44 μs

Unfortunately yes. I've done a benchmark of using module attributes vs. reading the application environment: Using module attributes are always faster. It's questionable whether the effects are acceptable. But I think faster is always better.

But using a Configuration module as a single point of truth IMHO has its charme. Therefore I think using a combination of module attributes within the Configuration module is the (my) way to go.

If someone need runtime configuration it's possible to pass a custom alphabet and size to the generate/2 function.

@florianb
Copy link

Ah yeah - that sounds pretty reasonable! Thank you very much for your work!

@railsmechanic
Copy link
Owner

I released v2.0.3 which now uses the Configuration module.

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

No branches or pull requests

3 participants