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

add relative path to component links in static builds #1062

Merged
merged 9 commits into from
Jul 19, 2021

Conversation

13twelve
Copy link
Contributor

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 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.

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 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"

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"
```
@mihkeleidast
Copy link
Member

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

@mihkeleidast
Copy link
Member

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.

The utils.relUrlPath is thoroughly covered with tests: https://github.com/frctl/fractal/blob/main/packages/core/test/utils.spec.js#L265

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.

@13twelve
Copy link
Contributor Author

Makes sense to move to the utils relUrlPath function. I added a relativeToCurrentFolder option incase of some future need to use/not use relative paths to the current folder. Passing this option through from the other adapters in the /packages/ folder. Also added new tests to test for usage of this new option and also updated the snapshots for the updated build paths with ./ prefixing the paths.

@13twelve
Copy link
Contributor Author

I'm now wondering if this relUrlPath should just be:

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 Uncaught DOMException: Blocked a frame with origin "null" from accessing a cross-origin frame. errors as Chrome treats frames and iframes as different origins when browsing local files. So, I'd argue the static builds need a web server and so the Fractal navigation can use absolute paths instead of relative ones. I'd imagine most people upload the Fractal build to a web server to share with their teams.

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 : in component handles I'll happily continue updating the method I've started, or switch to the simpler relUrlPath and update the tests accordingly.

@mihkeleidast
Copy link
Member

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.

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 :)

@13twelve
Copy link
Contributor Author

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.

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 base_path type thing for the build in the config but then you might need to have different base paths for the build folder locally and on your server and now this is sounding like a pain.

I'll look at changes for your earlier comments 👍🏻

13twelve added 2 commits July 15, 2021 11:11
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
@13twelve
Copy link
Contributor Author

@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

@mihkeleidast
Copy link
Member

@13twelve can you resolve the conflicts?

@13twelve
Copy link
Contributor Author

@mihkeleidast done 👍🏻

@13twelve
Copy link
Contributor Author

@mihkeleidast thanks for your time and patience! 🙌🏻

@mihkeleidast
Copy link
Member

Cheers, thanks for working on this!

@mihkeleidast mihkeleidast merged commit 2f84d3b into frctl:main Jul 19, 2021
ScottMorris added a commit to ScottMorris/fractal that referenced this pull request Jul 20, 2021
- The path generated to self will be emitted as ./self.ext
- This fixes an issue introduced by frctl#1062
mihkeleidast pushed a commit that referenced this pull request Jul 20, 2021
- The path generated to self will be emitted as ./self.ext
- This fixes an issue introduced by #1062
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.

2 participants