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

Convert site_address to site_addr #462

Merged
merged 4 commits into from
Feb 4, 2023
Merged

Convert site_address to site_addr #462

merged 4 commits into from
Feb 4, 2023

Conversation

benwis
Copy link
Contributor

@benwis benwis commented Feb 4, 2023

Should fix #446, by changing leptos internal options to site_addr

@gbj
Copy link
Collaborator

gbj commented Feb 4, 2023

  --> src/main.rs:47:31
   |
47 |     let addr = leptos_options.site_address;
   |                               ^^^^^^^^^^^^ help: a field with a similar name exists: `site_addr`

So this fails because this example uses leptos_options.site_address. We could get it to pass by changing the failing example but I imagine the starter templates and every app someone's created also uses site_address, so everyone's apps would suddenly stop compiling.

I think a serde alias that maps either site_addr or site_address to the site_address field might be better, to maintain that backwards compatibility.

@benwis
Copy link
Contributor Author

benwis commented Feb 4, 2023

If we were using serde for this, that'd be a great idea.

So if they're doing get_configuration(None), which should be all cargo-leptos users, cargo-leptos reads all the vars and leptos uses env vars. Since it's site-addr not site-address, the host/port is never set in leptos, and it uses the default. If someone changes it, leptos will silently use the default. It's a change on cargo-leptos' side to read both and set both env vars. I don't think we want to have leptos start reading the config in this configuration.

If they're using get_configuration(Some) with cargo-leptos, it would be working with site-address currently, since leptos is reading it correctly, and cargo-leptos is setting the wrong env var. They'll get an error with the change when they go to compile.

If they're using get_configuration(Some) without cargo-leptos, it would be working currently with site-address. They'll get an error with the change when they go to compile.

If they're using the builder, it would be working correctly now. They'll get an error with the change when they go to compile.

So this seems like the best solution to me.

@gbj
Copy link
Collaborator

gbj commented Feb 4, 2023

Isn't get_configuration(Some("Cargo.toml")) using serde, though? I see we derive Deserialize
https://github.com/benwis/leptos/blob/b14411029f8b1a968f8da3b0284606dd7f2214dc/leptos_config/src/lib.rs#L24

and then deserialize from the TOML
https://github.com/benwis/leptos/blob/b14411029f8b1a968f8da3b0284606dd7f2214dc/leptos_config/src/lib.rs#L160-L162

I'm probably missing something.

@benwis
Copy link
Contributor Author

benwis commented Feb 4, 2023

Technically we are, in that the config crate implements its traits and uses it to convert from the Config type to the ConfFile type, but the crate appears to validate and read the values from the file itself before it does so without serde. I tried various incantations of adding rename to the site_addr line, but none caused anything to change.

I see three possibilities here

  1. Leave leptos as is, change the examples back, and open a ticket to have cargo-leptos read site-addr and site-address as the same, and set the LEPTOS_SITE_ADDRESS env var.
  2. Change leptos to make it consistent with cargo-leptos and the examples with site-addr. People using the old value with cargo-leptos will get an error with the new version(and not a silent failure when they try to change the port), and people using the old value without cargo-leptos will get an error(although there's worked before) they'll have to fix
  3. Regex search and replace site-address in leptos with site-addr, and then special case reading the LEPTOS_SITE_ADDR env var and overwriting site_address

I lean towards 2 and away from 3 because it provides the most consistency, and it seems like we're ok with breaking changes. I can do 3 if you'd like, although I imagine we'd want to make the breaking change away pretty soon in the future anyway

@gbj
Copy link
Collaborator

gbj commented Feb 4, 2023

Yeah, Option 2 sounds good; our next release will be 0.2.0 because of some other breaking changes so this makes sense to me. The error Rust gives you when you try to access leptos_options.site_address instead of .site_addr is pretty useful anyway because the names are similar. And Option 2 is basically what your PR is at present, correct? So this should be ready to merge.

@benwis
Copy link
Contributor Author

benwis commented Feb 4, 2023

Correct, #2 is this PR and should be ready to go

@gbj gbj merged commit 76aeb57 into leptos-rs:main Feb 4, 2023
gbj pushed a commit that referenced this pull request Feb 12, 2023
gbj pushed a commit that referenced this pull request Mar 21, 2023
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.

site-addr, site_addr, site_address
2 participants