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

feat: 🎸 Use server.origin on build output #126

Closed
wants to merge 1 commit into from

Conversation

macabeus
Copy link

@macabeus macabeus commented Jan 24, 2021

Closes #120
Requires Elderjs/template#40

Now we can use the config server.prefix to the path prefix.
For example: on the route www.example.com/foo, the prefix is foo.

It's useful if you are developing a github/gitlab pages or a legacy application that just a route should work with Elder.js

@nickreese
Copy link
Contributor

nickreese commented Jan 25, 2021

@macabeus I like this PR in concept but I think there are quite a few places where this may break things as illustrated by the tests.

Will add comments on the code.


EDIT: Reading the code a bit closer it make sense what you did. Let me think about this in more depth.

@nickreese
Copy link
Contributor

nickreese commented Jan 25, 2021

As it currently stands I don't have a business case for this so I can't commit to writing this but I did spend about 45 minutes thinking through the code on how you'd achieve what you are looking for. I am happy to review a PR, but will insist on tests being robust.

Below is what I'd suggest we do to do a robust job of moving prefix out of the elder.config.js server object. Unfortunately from the surface this seems like a small change, but are quite a few things that need to happen.

Updating the Config and Handling Legacy Definitions:

  • 1. Move prefix out from within the server object and to it's own key on the root of the object within elder.config.js making the prefix accessible at (settings.prefix). This means updating validations.ts ... please note that validations.ts is used in building the documentation.
  • 2. Normalize the prefix to always be structured as /prefix/goes/here without the trailing slash as when combined with permalinks this should result in a fully qualified URL.
  • 3. I believe the the clientComponents variable defined in getConfig.ts which is passed into prepareFindSvelteComponent will need to include the prefix as this controls where the svelte component files are written to. (We need to consider how windows relative file paths may not work correctly with \ and /. )
  • 4. Similarly to 3, distElder will need to include the prefix if present as this controls where lots of things are written to especially within rollupPlugin.ts where we are copying over files. (We need to consider how windows relative file paths may not work correctly with \ and /. )
  • 5. To prevent a breaking change, check for settings.server.prefix in getConfig.ts and if that key is found, set it to settings.prefix
  • 6. Update corresponding types to depreciate settings.server.prefix
  • 7. Update config.$$internal.publicCssFile to include the prefix.
  • 8. To avoid confusion we should rename the current config.$$internal.prefix which is used for console logging.

Implementing the Prefix Change

  • 1. Once we have a new location where we know prefix will always be (settings.prefix), we should adjust wrapPermalinkFn to include the prefix so that the usual way of doing permalink within templates works. This allows for helpers.permalinks.blog({slug: "foo"}) to include the prefix automatically... This solves the issue shown in feat: 🎸 Use server.origin on build output template#40 (review)
  • 2. To avoid duplication of the prefix, we should edit permalinks.ts to remove the adding of prefix. (This was wedged in before I open sourced Elder.js).
  • 3. To avoid duplication of the prefix, we should also remove lines 316-318 in Elder.ts because our update to the wrapPermalinkFn should handle this logic.
  • 4. The hook elderExpressLikeMiddleware within hooks.ts will need to be updated to handle the new prefix location.
  • 5. The hook elderAddDefaultIntersectionObserver within hooks.ts needs to be updated to handle the new prefix location.

Tests

This is quiet a large change so in addition to getting the current tests passing we should have tests that cover at a minimum:

  • Prefix beginning with a slash (getConfig.ts)
  • Prefix ending with a slash (getConfig.ts)
  • Prefix with both slash at beginning and end. (getConfig.ts)
  • No prefix (the default) (getConfig.ts)
  • Testing of the prefix and without a prefix (permalinks.ts)
  • Test to cover old settings.server.prefix usage. (getConfig.ts)
  • Test to make sure that regardless of how the prefix is set that settings.$$internal.elderDir always works on windows and linux.
  • Test to ensure that the hook for ``elderExpressLikeMiddleware` works with all variations of prefixes.
  • Test to ensure that the hook for ``elderAddDefaultIntersectionObserver` works with all variations of prefixes.

@macabeus
Copy link
Author

Closing in favour of #128

@macabeus macabeus closed this Jan 29, 2021
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.

Origin isn't being used to prefix the links
2 participants