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.

Sign up for GitHub

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

Jump to bottom

Handle the base in the Path module #230

Closed
wants to merge 2 commits into from
Closed

Handle the base in the Path module #230

wants to merge 2 commits into from

Conversation

j-maas
Copy link
Contributor

@j-maas j-maas commented Sep 14, 2021 •
edited
Loading

This fixes #217.

I was playing around with the code to see if I could fix that issue. The solution I ended up with is actually close to what Dillon and I discussed on slack regarding how the base is handled.

With this PR that the Path.elm is generated to allow inlining the baseUrl at compile-time and make Path.toAbsolute (which prepends the baseUrl) synchronous. I left the baseUrl in Route.elm, because of the Route.urlToRoute function that uses it.

This change cleared up my mental model of how to use paths: Just store everything as a Path (via e.g. Path.fromString or Route.toPath) and when I need to have the URL as a String to paste into e.g. a src attribute, I call Path.toString on it. Therefore Paths can be anything that is located on my site.

I've marked this as a draft, because I'm not sure if this solution is what you, Dillon, had in mind. Also there are some things left to do, like the <head> tags not having the base prepended and the tests have to be updated.

@j-maas
Handle the base in the Path module
b118e4f 
We now generate the `Path` module to allow inlining the `basePath` at compile-time.
@j-maas
Copy link
Contributor Author

j-maas commented Sep 15, 2021 •
edited
Loading

@dillonkearns If this is the right direction to go into, feel free to tell me. 😊 I'll see if I'll find some time to work on this more.

@j-maas
Remove trailing slash
48daf6c 
A trailing slash can lead to issues when the path is to e.g. an `.svg`.
@dillonkearns
Copy link
Owner

@y0hy0h thinking out loud a bit, some pros and cons are:

Pro

  • It's convenient to use a generated Path module
  • It does the right thing if you try to build up a Path (doesn't point you to a URL without a base)

Cons

  • It's a little implicit and may be surprising (maybe users might expect that they need to add the base since it's added implicitly?)
  • The docs would live locally, not on the package site
  • Published code can't depend on the Path module

One other possible direction could be to generate something like a Pages.base : BasePath. Then getting Path.toAbsolute (or toString) would require passing in that value. That would sort of have the reverse problem compared to the pros and cons above.

I'm traveling at the moment so I haven't had a chance to think about it much, but that's just a brain dump to get the thoughts flowing a bit. Any thoughts on all that?

@j-maas
Copy link
Contributor Author

j-maas commented Sep 18, 2021

@dillonkearns Thanks for your feedback! Take your time traveling and respond when it's convenient to you! :)

I realize that I mixed up two things in Path:

  • It takes care of constructing a valid path without the need to worry about slashes, and
  • it provides a way to point to a part of your elm-pages site, abstracting away the base so that you can configure the base at build-time.

I agree that it's cleaner to separate the two. Since that slash-handling is quite common and you might want to refer to it from published code, Path should probably stay as it currently is.

To solve the second issue, a Pages.base and especially something like a Pages.pathToAbsoluteUrl : Path -> String would be great! The only use case I can think of where you're concerned about the base is to construct URLs, so that should take care of that.

Since people who want to configure the base will need to use the --base flag, we also have a good place to point people to a Pages.pathToAbsoluteUrl method. If they haven't used that method before, they might need to migrate some call sites, but it should be a straight-forward migration.

I'll have a go at that approach and open a new PR, so that we're able to compare them more easily.

@j-maas j-maas closed this Sep 18, 2021
@dillonkearns
Copy link
Owner

Sounds great, thank you @y0hy0h!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

elm-pages build --base sub does not render correctly
2 participants
@j-maas @dillonkearns