Extracted url-utils package from Ghost core

[naz]

refs TryGhost/Ghost#10773

Adds @tryghost/url-utils module that is equivalent to current Ghost Url Service utils with exception to couple usage changes:

1. The module has to be initialized through the init(params) method - this allowed to decouple it from the core's config dependency
2. replacePermalink - now accepts additional parameter - timezone which allowed to remove the dependency on Settings Cache.
3. Test cases were refactored to suit new initialization style and assertions were not changed

@rishabhgrg @kevinansfield would love to hear your opinions on these changes. Mainly wanted to double check on this additional initialization method and naming of the parameters it takes in.

To expand a bit on how initialization would work now. I'm thinking to have it initialized on server bootstrap right after the server configuration is set up. It gives one additional step but couldn't think of any nicer way of doing so.

TODO:

• leave a comment about future refactoring and current issues that prevented from doing so now (Refactor url-utils package #117)

[rishabhgrg]

@gargol Looking at all the ways we are using config in the file and the init method, I feel there are couple of ways this can re-structured -

1. If the expectation to use util methods is to have initialization with config before using them, then probably its better to either wrap the methods as class/constructor or a factory/closure approach and expose that single method with config as parameter, as it explicitly forces developer to pass/initialize with valid config before using any of util methods uses config implicitly. The passed config can be checked once in init to be valid, and fail during initialization if not, allowing developer is free to use methods as pure fns worrying just about passing right params.
   Con : Methods which don't use config also go behind initialization, and can't be used directly with just params.

2. While looking at methods use config, it seems each of them mostly use only one particular config, and not multiple. If we want to expose each util method individually, then its probably better to pass the config property being used as a param to the method, instead of having a config object for whole of utils. This allows each method to be exposed individually as pure fns, and also exposes method which don't use config to be used directly by developers.
   Con : Config needs to be fetched and passed as param wherever util method is used, instead of initializing it once( maybe at boot ) and then just using the utils as a service directly.

I feel either approach, though have their own cons, resolves the ambiguity of config being passed to util method. I am generally inclined towards second as a developer because of freedom to choose utils which don't have config, but looking from Ghost's perspective, its probably better to do first where utils is used as a service which is initialized once and then used everywhere. 😄

[naz]

Awesome feedback, thanks! 💯 agree about the 2nd approach being the best.

My main concern was having to add config dependency in every place where functions are called in Ghost. For example for getSubdir method around 8 out 12 usages of this method would have to require a config dependency + it would have to be passed in as a parameter for each internal method that uses getSubdir (almost all internal utils methods). It would be a lot of shuffling around and adding dependencies to have pure functions in this util without having at least blogs url preconfigured.

This brings us to the 1st approach. Think it is the least intrusive approach and makes sense to have it hidden behind a constructor to have more explicit "you gotta initialize this thing before using". Will experiment with this approach 👍

[rishabhgrg]

@gargol Yeah I agree that second approach is too cumbersome for our primary usecase, and while first can still be made flexible enough like (2) with slightly more work where even if you pass empty config, it allows you to use methods which don't require config, while throwing error for ones that do.

Just FYI that For (2), there is a non constructor/class as well if it suits better, where it takes advantage of JS closure to give us methods which has config initialized, without having do a proper url utils class setup. Either ways the instance returned will need to be preserved to use all around core. 😄
Roughly:

function UrlUtils(config) {

   return {
       method1: () => {},
       method2: () => {}
       ....
   }
}

export default UrlUtils;

[naz]

@rishabhgrg @kevinansfield pushed out changes to the initialization of the module and have extracted a few methods into separate modules (the ones that were pure functions).

The rest, unfortunately, is very tied up to config.url value behaving like a "global" in some methods (specifically in getSubdir). Didn't figure out a nice way to abstract that away without running into circular dependency hell, would table it for future improvements? Or maybe you have some ideas on how to work around it? 🤔

[rishabhgrg]

@gargol I think a nice way to extract out methods to separate file as well as being able to use global config could be what we were thinking about doing with approach (2) here. All the methods can be put in separate file(s) taking config as an extra parameter, making them all pure methods, which allows them to be imported in other files as well as index.js. So if getSubdir, is pure method which takes config as extra param and returns its value, in our index getSubdir we can just wrap that method to pass it our global config, while in a method like deduplicateSubDir we can require getSubdir pure method and pass it config, which does not make a circular dependency.

The advantage with this approach is it adds all the benefits of (2) where util methods are pure methods ready to be exported when needed in future, while taking care of its main con where config needed to be passed everywhere in ghost

main concern was having to add config dependency in every place where functions are called in Ghost.

as it still has advantages of (1) where it can easily be initialized at once place with config. 🤔

In getSubdir file -

module.exports = function getSubdir(config) {
        // Parse local path location
        var localPath = url.parse(config.url).path,
        ...
        return subdir;
}

In deduplicateSubDir file -

const getSubdir = require('./getSubdir');
...
module.exports = function deduplicateSubDir(url, config) {
        var subDir = getSubdir(config),
        ...
       return url.replace(subDirRegex, '$1' + subDir + '/');
}

In index.js -

const getSubdir = require('./getSubdir');
const deduplicateSubDir = require('./deduplicateSubDir');
...
utils.getSubdir = () => getSubdir(config)
utils. deduplicateSubDir = (url) => deduplicateSubDir(url, config)
...

[naz]

I tried the approach of passing the parameter to make the functions pure. The problem with such refactoring starts when there are couple nested calls from method to method. The main one, afair, was with 'join urls' which operates on arguments directly :/ If I pass in a config string 'url' to it, to pass later to getsubdir, it's impossible to distinguish without introducing some weird convention like 'first parameter will always be a url'. I'll check back about the details once I'm around the computer :)

…

On Fri, Jun 7, 2019, 05:47 Rishabh Garg ***@***.***> wrote: @gargol <https://github.com/gargol> I think a nice way to extract out methods to separate file as well as being able to use global config could be what we were thinking about doing with approach (2) here <#114 (comment)>. All the methods can be put in separate file(s) taking config as an extra parameter, making them all pure methods, which allows them to be imported in other files as well as index.js. So if getSubdir, is pure method which takes config or url as param and returns value, in our index getSubdir we can just wrap that method to pass it our global config, while in a method like deduplicateSubDir we can require getSubdir pure method and pass it config, which does not make a circular dependency. The added advantage with this approach is it retains all the benefits of (2) where util methods are pure methods ready to be exported when needed in future, and also still has advantages of (1) where it can easily be initialized at once place with config. 🤔 In getSubdir file module.exports = function getSubdir(config) { // Parse local path location var localPath = url.parse(config.url).path, subdir; // Remove trailing slash if (localPath !== '/') { localPath = localPath.replace(/\/$/, ''); } subdir = localPath === '/' ? '' : localPath; return subdir; } In deduplicateSubDir file: module.exports = function deduplicateSubDir(url, config) { var subDir = getSubdir(config), subDirRegex; ..... } In index.js - const getSubdir = require('./getSubdir'); const deduplicateSubDir = require('./deduplicateSubDir'); ... utils.getSubdir = () => getSubdir(config) utils. deduplicateSubDir = (url) => deduplicateSubDir(url, config) .... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#114>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFE4ROOYHIN7W3P4GNXXQTPZHK3XANCNFSM4HT4KQ6A> .

[rishabhgrg]

@gargol Ah, I see what you mean! 🤔 Yeah that will mean we'll need to redefine joinUrl to take first parameter as config (we can skip doing specific config and just use config as whole) and rest as urls, probably something like joinUrl(config, ...rest). which technically make sense in context of ghost as joinUrl should know config.

tbh though I don't think we need to spend too much time to do this now, all of this can be done as part of cleanup later, we can just leave a comment and move on for now. Wdyt? 😄

[naz]

Yes, leaving a note or even better opening up an issue would let us keep to the main goal. This wasn't meant to be a full blown refactor - just an extraction with quick possible improvements :) Lmk if there are any other things to discuss. Would merge this in and integrate it with the core today. Wdyt?

…

On Fri, Jun 7, 2019, 07:13 Rishabh Garg ***@***.***> wrote: @gargol <https://github.com/gargol> Ah, I see what you mean! 🤔 Yeah that will mean we'll need to redefine joinUrl to take first parameter as config (we can skip doing specific config and just use config as whole) and rest as urls, probably something like joinUrl(config, ...rest). which technically make sense in context of ghost as joinUrl should know config. tbh though I don't think we have to do this now, all of this can be done as part of cleanup later, we can just leave a comment and move on for now. Wdyt? 😄 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#114>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFE4RNIWCA7LLNPIZ5DU2TPZHU67ANCNFSM4HT4KQ6A> .

[rishabhgrg]

Would merge this in and
integrate it with the core today. Wdyt?

@gargol Go for it 👍