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

support deployment to subdirectories #40

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jelhan
Copy link

@jelhan jelhan commented Mar 13, 2020

Closes #19

@mansona
Copy link
Member

mansona commented Mar 13, 2020

Hey @jelhan 👋 thanks for your interest in this. Unfortunately it's not quite as simple as just moving the rootUrl because of the nature of the "static api" that also gets deployed at the same time.

I think I'll open a discussion on #19 to get into more of an understanding of why this is a good feature to think about and we can then figure out the best way forward 👍

@jelhan
Copy link
Author

jelhan commented Mar 13, 2020

Unfortunately it's not quite as simple as just moving the rootUrl because of the nature of the "static api" that also gets deployed at the same time.

Can you point me to the parts that are broken? I haven't done much testing yet to be honest but it seem to work on the first try. At least I didn't noticed any issue when clicking around in a deployed app on a subdirectory. I only see a 404 for versions.json but I thought that's expected as I haven't done any tagged release yet on testing repository.

@jelhan
Copy link
Author

jelhan commented Jul 6, 2020

@mansona Any chance to move forward here? You mentioned that it's not that simple as I have done it here. Could you please share your concerns. I'm happy to work on the issues left if you are willing to accept such merge requests. To be honest I'm not sure how much work is left. I'm using guide maker deployed to a subfolder since a few month now. Didn't faced any issue so far.

@mansona
Copy link
Member

mansona commented Jul 6, 2020

hey @jelhan thanks for the ping 👍

So it turns out that I have recently solved this issue with another Empress product called Field Guide 🎉 and one of the issues that the change you are making is being shown in the netlify failed build at the moment. If you take a look you will see that the prember build is unable to find some of the JSON files request to http://content/versions.json failed and this is because prember didn't listen to the rootURL for static files.

This has been fixed in ef4/prember#57 5 days ago 😂 🎉

so, I guess I have a few questions that would help me progress this PR: firstly you say that you've been using it for a few months and you haven't noticed any issues 🤔 does that mean that you were not making use of the prember functionality and just relying on the SPA behaviour? Secondly, which template are you using? I have noticed in my Field Guide experiments that some of the paths to icons and images need to also make sure they are using the rootURL and I haven't yet found a perfect solution to that, and I'm curious to know if you have had similar issues?

@jelhan
Copy link
Author

jelhan commented Jul 6, 2020

So it turns out that I have recently solved this issue with another Empress product called Field Guide

Awesome news! 🎉

firstly you say that you've been using it for a few months and you haven't noticed any issues thinking does that mean that you were not making use of the prember functionality and just relying on the SPA behaviour?

Yes. As the documentation created with it is internal only and not that much used I wasn't caring about that one. Actually I didn't noticed until now that it should use Prember. But that should be resolved now if I got you right, isn't it?

Secondly, which template are you using? I have noticed in my Field Guide experiments that some of the paths to icons and images need to also make sure they are using the rootURL and I haven't yet found a perfect solution to that, and I'm curious to know if you have had similar issues?

I'm using the default template. Faced the same issues as you describe. I fixed them on a feature branch. You could find that merge request here: https://github.com/empress/guidemaker-default-template/pull/6/files

For configuration I'm currently relying on duplicating the root URL if needed. That might not be the best solution. We might instead prefix guidemaker.logo URL with rootURL as well. This would require some additional changes to the default template.

@mansona
Copy link
Member

mansona commented Jul 6, 2020

so.. I just read your argument in empress/guidemaker-default-template#6 about adding root-url helper being a breaking change and I'm not 100% sure that I agree 🤔 currently it would not cause a breaking change (or rather throw an error) unless Ember decided to include root-url as a default helper (similar to the issues with the array helper)

I'm also thinking that rootURL should be supported on a Guidemaker level and not something that should be individually supported by each of the templates 🤔 which gets me thinking that we should make root-url a dependency of this repo and not guidemaker-default-template. What do you think?

Also I am curious what you mean by For configuration I'm currently relying on duplicating the root URL if needed 🤔 can you give an example of where this matters? The only ones that I can think of are things that you would usually only care about if you have prember installed (SEO related)

@jelhan
Copy link
Author

jelhan commented Jul 7, 2020

so.. I just read your argument in empress/guidemaker-default-template#6 about adding root-url helper being a breaking change and I'm not 100% sure that I agree thinking currently it would not cause a breaking change (or rather throw an error) unless Ember decided to include root-url as a default helper (similar to the issues with the array helper)

I meant that adding a controller to calculate the root URL might be a breaking change. I don't see an issue by adding this helper.

I'm also thinking that rootURL should be supported on a Guidemaker level and not something that should be individually supported by each of the templates thinking which gets me thinking that we should make root-url a dependency of this repo and not guidemaker-default-template. What do you think?

That would help with having a harmonized ecosystem for sure. On the other hand it might not be needed for custom templates that don't care about subdirectory support. I don't have a strong opinion on that one.

Also I am curious what you mean by For configuration I'm currently relying on duplicating the root URL if needed thinking can you give an example of where this matters?

I was especially referring to the guidemaker.logo configuration. This should be a URL pointing to an image. It's up to discussion if that URL should include the root URL or if it will be prefixed with it when used:

If it's not prefixed automatically the configuration and usage in template would look like this:

{
  rootUrl: '/foo/',
  guidemaker: {
    logo: '/foo/images/logo.jpg'
  }
}
<img src={{this.guidemaker.logo}} />

This is what I'm currently having.

Alternatively we could prefix guidemaker.logo with root URL automatically:

{
  rootUrl: '/foo/',
  guidemaker: {
    logo: '/images/logo.jpg'
  }
}
<img src={{root-url this.guidemaker.logo}} />

But it's not as simple as this cause absolute URLs that include a domain must not be prefixed with root URL. So this would add some complexity.

@mansona
Copy link
Member

mansona commented Jul 7, 2020

I think the most consistent approach will be to make sure that we use the {{root-url}} helper in all cases in the template 🤔

But it's not as simple as this cause absolute URLs that include a domain must not be prefixed with root URL

What do you mean by this? 🤔 are you saying that you might expect people to put something like this in the config:

{
  rootUrl: '/foo/',
  guidemaker: {
    logo: 'https://emberjs.com/images/ember-logo.svg'
  }
}

because I would consider this an error 🤔 and maybe a major part of fully supporting rootURL properly is just putting warnings and linting in place that makes sure that people are doing things correctly 👍

@jelhan
Copy link
Author

jelhan commented Jul 7, 2020

thinking are you saying that you might expect people to put something like this in the config:

{
  rootUrl: '/foo/',
  guidemaker: {
    logo: 'https://emberjs.com/images/ember-logo.svg'
  }
}

Yes. I thought that this is supported. Treating this as not supported will make implementation way easier. 👍

@mansona
Copy link
Member

mansona commented Jul 7, 2020

I think it might be accidentally supported at the moment 🤔 but we can definitely warn against it for now 👍

@jelhan
Copy link
Author

jelhan commented Jul 7, 2020

I think the most consistent approach will be to make sure that we use the {{root-url}} helper in all cases in the template thinking

Would this include images referenced in the markdown as well?

![some image](/images/foo.jpg)

And what about HTML embedded in the markdown? Would we support some kind of {{root-url}} to be used inside of markdown?

@mansona
Copy link
Member

mansona commented Jul 7, 2020

hmm... 🤔 now we're getting into tricky territory 🤔

I think in the case of markdown image links you might be able to get away with relative urls 🤔 but I know this is probably not ideal. On the other hand, you should be able to use {{root-url}} directly in the markdown and have it work 😂 at least for embedded html that should work.

Can you try <img src="{{root-url /images/foo.jpg}}"> in your current setup?

@jelhan
Copy link
Author

jelhan commented Aug 4, 2020

I'm sorry that I missed to follow-up here. Was pulled into other topics.

I think in the case of markdown image links you might be able to get away with relative urls thinking but I know this is probably not ideal. On the other hand, you should be able to use {{root-url}} directly in the markdown and have it work joy at least for embedded html that should work.

Can you try <img src="{{root-url /images/foo.jpg}}"> in your current setup?

I tried but the helper does not run.

helper in markdown

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.

Work with rootURL other than /
2 participants