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

Adds support for client subdir #1516

Merged
merged 18 commits into from
Oct 27, 2023
Merged

Adds support for client subdir #1516

merged 18 commits into from
Oct 27, 2023

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Oct 17, 2023

Closes #8

You can now serve the client from a subdirectory. This is useful if you want to serve the client from a subdirectory of your domain, e.g. https://example.com/my-app/.

To do this, you need to add the client.baseDir property to your .wasp file:

app todoApp {
  // ...
  client: {
    baseDir: "/my-app",
  },
}

},
title: "subdir-example",
client: {
baseDir: "/bla"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of usage

@infomiho infomiho marked this pull request as ready for review October 18, 2023 11:57
Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed, looks good!

I left some smaller comments, feel free to fix them as you wish and you can merge when you are satisfied.

Some additional questions, but also not critical:

  1. How did you choose baseDir for a name? I see Vite uses base, react-router uses basename. I like baseDir the most probably, I think it is the most precise, I am just curious.
  2. Idea: what if they need to use the value of "baseDir" in frontend/backend? I can see that being possible? Should we expose it for them, so they can just import it and use it, since Wasp already knows it? Instead of them duplicating it in shared/?

waspc/src/Wasp/AppSpec/App/Client.hs Show resolved Hide resolved
waspc/src/Wasp/AppSpec/Valid.hs Outdated Show resolved Hide resolved
waspc/src/Wasp/Project/WebApp.hs Outdated Show resolved Hide resolved
@infomiho
Copy link
Contributor Author

Note: check out the email sending code that relies on the frontend URL for email verification and password reset. URL in the config probably needs updating. Also the docs need to be updated.

@Martinsos
Copy link
Member

Note: check out the email sending code that relies on the frontend URL for email verification and password reset. URL in the config probably needs updating. Also the docs need to be updated.

note: docs should be updated to mention that frontend URL needs to contain that same base dir in it. I wonder if we could even do a check for that in Wasp.

waspc/src/Wasp/Generator/WebAppGenerator.hs Outdated Show resolved Hide resolved
waspc/src/Wasp/Project/WebApp.hs Outdated Show resolved Hide resolved
web/docs/project/_baseDirEnvNote.md Outdated Show resolved Hide resolved
@Martinsos
Copy link
Member

Note: check out the email sending code that relies on the frontend URL for email verification and password reset. URL in the config probably needs updating. Also the docs need to be updated.

How are with the email then, all good?

@Martinsos
Copy link
Member

Reviewed, looks good!

I left some smaller comments, feel free to fix them as you wish and you can merge when you are satisfied.

Some additional questions, but also not critical:

  1. How did you choose baseDir for a name? I see Vite uses base, react-router uses basename. I like baseDir the most probably, I think it is the most precise, I am just curious.
  2. Idea: what if they need to use the value of "baseDir" in frontend/backend? I can see that being possible? Should we expose it for them, so they can just import it and use it, since Wasp already knows it? Instead of them duplicating it in shared/?

@infomiho bump on this one, some questions here!

@infomiho
Copy link
Contributor Author

  1. The baseDir name is arbitrary. It felt right given the base and basename nomenclature.
  2. I wouldn't add it unless we get explicit requests for it, keeping it simple for now. baseDir is already an edge case, so exposing it an even more of an edge case IMHO
  3. Email links now work as expected 👍 I've tested it out locally

@infomiho infomiho requested a review from Martinsos October 25, 2023 11:04
@Martinsos
Copy link
Member

  1. The baseDir name is arbitrary. It felt right given the base and basename nomenclature.
  2. I wouldn't add it unless we get explicit requests for it, keeping it simple for now. baseDir is already an edge case, so exposing it an even more of an edge case IMHO
  3. Email links now work as expected 👍 I've tested it out locally

Adding baseDir in SDK -> I don't see why that is more of an edge case, it seemed logical to me to give them a way to access that value they defined in .wasp file. Otherwise they will have to duplicate it. They might need it to construct some kind of URL maybe?

@infomiho
Copy link
Contributor Author

infomiho commented Oct 25, 2023

I don't feel we have a strong argument for adding it.

Otherwise they will have to duplicate it. They might need it to construct some kind of URL maybe?

When users start mentioning they need it -> I would then add it. Otherwise, we are adding things we assume could be useful .

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infomiho approved! There might be some comments that are still somewhat unresolved, pls make sure those are all in good shape, and then you can merge!

]

configFileInSrcDir :: Path' (Rel C.ServerSrcDir) File'
configFileInSrcDir = [relfile|config.js|]
configFileInSrcDir :: Path' (Rel C.ServerSrcDir) File'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this one get indented, if it wasn't indented before? It makes sense to me that it is at the top level, since it says where the config file is, in theory some other part of Generator might want to use that info.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that it was used only in one place, therefore I moved it inside of the genConfigFile

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say it is not about how it is used currently, it is about how it is intended to be used. We keep all of such paths at top level (if I am correct), because they are often by other modules for imports and similar. You putting into where indicates you mean it to be private/local for this function, so if I want to use it later, I will have to wonder if I can use that one -> it seems like the author didn't intend so.

So I would leave where it was.

Copy link
Contributor Author

@infomiho infomiho Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍 I've reverted the change

waspc/src/Wasp/Generator/WebAppGenerator/Common.hs Outdated Show resolved Hide resolved
@infomiho infomiho requested a review from Martinsos October 25, 2023 13:51
@Martinsos
Copy link
Member

I don't feel we have a strong argument for adding it.

Otherwise they will have to duplicate it. They might need it to construct some kind of URL maybe?

When users start mentioning they need it -> I would then add it. Otherwise, we are adding things we assume could be useful .

Fair enough -> personally, to me it just makes a lot of sense to have it, and at the end we are always assuming that somebody will use something :D, the question is just how certain are we of it. What is tricky here is that people might not ask for it but just duplicate it. But ok, let's not get stuck on it, we can certainly add it later as you said, hopefully we will be the ones to stumble upon needing it first, if it is indeed useful.

@infomiho infomiho merged commit 520c35a into main Oct 27, 2023
4 checks passed
@infomiho infomiho deleted the miho-client-subdir branch October 27, 2023 13:12
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.

Add support in Wasp for hosting the frontend in non-root directory
2 participants