-
Notifications
You must be signed in to change notification settings - Fork 172
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
add relative path to component links in static builds #1062
Conversation
In a static build the links to components now begin with `./` incase a user has set a handle with a `:` in it. Having a `:` in your component handle would produce a static url like: `href="foo:btn.html"` The browser tries to interpret this as an app link, which gives you a `Failed to launch 'foo:btn' because the scheme does not have a registered handler` error message in the browser developer console and stops page navigation. Having a relative path current folder `./`, so producing `href="./foo:btn.html"` stops the browser interpreting as an app link. For server builds, this doesn't apply, the existing paths are used as before. Why use `:` in a component handle? Components may naturally have `:` in their name, such as `ratio-box--16:9` or a user maybe name spacing components in a multisite Fractal library, where components may have the same name but live in different sites: ``` {% include '@foo:btn' with data %} // outputs `btn` from site `foo`, "handle": "foo:btn" {% include '@bar:btn' with data %} // outputs `btn` from site `bar`, "handle": "bar:btn" ```
The adapters implement their own version of the path helper. Do you think those should also behave the same way? For example, Handlebars adapter does the same thing here: https://github.com/frctl/fractal/blob/main/packages/handlebars/src/helpers/path.js#L32 |
The Maybe adding the logic inside that function would be better? Then it would also automatically cover the adapters as well, plus would be covered by tests (since adding it outside of that function is not covered by anything I think) and we could add more tests to validate your problem cases. |
so we can optionally choose to have a relative path from the current folder or not
Makes sense to move to the utils |
I'm now wondering if this relUrlPath(toPath, fromPath, opts) {
return Path.extname(toPath).length > 0 || toPath.startsWith('http') || toPath.startsWith('.') || toPath.endsWith('/') ? toPath : toPath + (opts.ext || '');
} If everything uses absolute paths and we just add the file extension if needed for static builds then links should always work. I guess the use case for the relative paths was so that users could do a Fractal build and then double-click the generated index file and navigate around without a web server. But, I'm not sure this is needed. It mostly works but you can't flip between the various info panels for a pattern, at least in Chrome, because you hit Or, maybe it is still useful to be able to browse a Fractal pattern library locally without needing a web server and this just comes with the caveat that you can't swap the info panels when browsing a pattern (I guess the fix would be a page refresh?). @mihkeleidast what do you think? As I'm keen to allow |
Not necessarily, the relative paths also allow users to upload the static build to any subdirectory on a server and have things just work. I use it daily in my work so let's not remove that functionality :) |
Ah yes, fair point. I guess we could have a I'll look at changes for your earlier comments 👍🏻 |
and revert various path/adapters to previous state, as they all read `app.web.get('builder.urls')` and so get the new `builder.urls.relativeToCurrentFolder` option
Tests updated to match
@mihkeleidast updates made - I prefer it with the config setting like this. I guess once done, the Fractal.build Web UI Configuration page needs a new entry |
@13twelve can you resolve the conflicts? |
@mihkeleidast done 👍🏻 |
@mihkeleidast thanks for your time and patience! 🙌🏻 |
Cheers, thanks for working on this! |
- The path generated to self will be emitted as ./self.ext - This fixes an issue introduced by frctl#1062
- The path generated to self will be emitted as ./self.ext - This fixes an issue introduced by #1062
Purpose of update:
Having a
:
in your component handle would produce a static URL like:href="foo:btn.html"
The browser tries to interpret this as an app link, which gives you a
Failed to launch 'foo:btn' because the scheme does not have a registered handler
error message in the browser developer console and stops page navigation.Having a relative path current folder
./
, so producinghref="./foo:btn.html"
stops the browser interpreting as an app link.For server builds, this doesn't apply, the existing paths are used as before.
I can't lie, changing paths in the build of a project I'm not super familiar with gives me the fear. The tests pass, although I'm not sure anything tests following links in static builds.
What are app links?
Firstly, apologies if you're already familiar, I wasn't, or at least, I wasn't aware that beyond
tel://
they are a thing. To open the Twitter app from a link if you have it installed<a href="twitter://user?screen_name=opticalcortex">open</a>
.It seems the browser just needs the
:
and not the://
to interpret a link as an app link. And so using:
in a Fractal handle, the browser fails to find an appropriate app and so emits an error message in the console and stops.Why use
:
in a component handle?Components may naturally have
:
in their name, such asratio-box--16:9
or a user maybe name spacing components in a multisite Fractal library,
where components may have the same name but live in different sites: