Expose resolvePath

[fatfisz]

As discussed in #189, there is a need for exposing some of the internal methods (importing from package/lib/* is not really a future-proof solution, because this is an undocumented API).

Also normalizeOptions is rewritten to prevent modifying the passed options object.

[codecov]

Codecov Report

Merging #194 into master will not change coverage.
The diff coverage is 100%.

Impacted Files	Coverage Δ
src/resolvePath.js	100% <100%> (ø)
src/transformers/call.js	100% <100%> (ø)	⬆️
src/utils.js	100% <100%> (ø)	⬆️
src/normalizeOptions.js	100% <100%> (ø)	⬆️
src/index.js	100% <100%> (ø)	⬆️

[fatfisz]

I'm adding the docs, will need a review of them.

[tleunen]

Thanks, I'll review this tomorrow.

[tleunen]

Do you think it make sense to expose normalizeOptions?
I wonder if we should change our logic to make getRealPath take the initial option, or maybe just export a new function (resolvePath?) that will normalize and then use our internal getRealPath?

I'm just saying that because normalizing is really internal to our lib (mostly because of the custom cwd values and alias regexes).

[fatfisz]

Makes sense. Let me sleep on that :)

[fatfisz]

Ok, I think I'm for including normalization in getRealPath and renaming it to resolvePath instead of adding another function. What do you say?

[tleunen]

Yep we can do that too

[fatfisz]

It turned out that the options are also used before resolvePath (in the call transformer, for the methods) and so I had an idea to put the normalized options in the state and pass the state as this. If there is normalizedOpts on this, then resolvePath uses it; else it computes normalizedOpts itself. The second scenario might happen if the function is called in the userland.

[fatfisz]

Both cases were already covered by the tests, so I didn't have to add any 😉

[fatfisz]

@tleunen If you're ok, I'd go ahead and merge it.

[tleunen]

I'm checking it that now. Thanks for the update @fatfisz

[tleunen]

I'm not sure I'm a fan of using resolvePath with a state context.

I believe I'd prefer that the function we export for the external use would be a new one that normalize and then use the internal resolvePath.

In a way, it feels weird to have resolvePath requiring to be called on a speficic context, it should be stateless.

Let me know what you think, we can discuss about it :)

[fatfisz]

Technically it can be stateless; this is just a crutch so that the original resolvePath can reuse the normalized options if they are there. Without state as this the function will just normalize by itself.

I agree that it may be over-optimized (normalizing seems to be quite cheap). Do you think it's better to get rid of this for clarity?

[tleunen]

I was thinking of calling it with resolvePath(sourcePath, currentFile, state.normalizedOpts).

And then the function we export in index.js would be a custom function doing:

 export { 
  resolvePath: (sourcePath, currentFile, opts) => {
    const normalizedOpts = normalizeOptions(currentFile, opts);
    return resolvePath(sourcePath, currentFile, normalizedOpts);
  }
 };

[fatfisz]

But later (with the new resolvePath option) this call will need opts as opposed to normalizedOpts:

const modulePath = opts.resolvePath(sourcePath, currentFile, opts);

[fatfisz]

I'll go ahead and eliminate the context then, since we seem to agree on that 😃

[fatfisz]

I think there's a value in having the same singature for the internal resolvePath and the external one; also options are being normalized specifically because of the needs of resolvePath, so it makes sense to just put it inside instead of exporting a wrapper function. What do you think?

[tleunen]

I see your point, but forcing to normalize the options again and again might impact the performance, especially since it might not be needed.

The pre hook was done on a per file basis, am I right? I never remember that and I can't find the doc on that now.
But if it's the case, we'll normalize the options once, and then multiple times for each import/require statement.

If this is really the way we want to go, to keep the same signature for the internal and external resolvePath, we'd need to add memoization on the normalize fn.

[fatfisz]

Yes, I was thinking the same thing - that's why I've added the state as this, to get the already normalized options from it. But maybe applying a more straightforward memoization is better? I think lodash.memoize could do it.

[tleunen]

Yes, and I believe we could also optimize normalizeOptions and especiallynormalizeCwd if instead of passing the currentFile, we actually use path.dirname(currentFile), don't you think?
That way, we'll really gain benefit from the memoization, since all files from the same directory will reuse the same normalized options.

[fatfisz]

Brilliant!

[fatfisz]

I settled on reselect because it required less configuration to use (lodash.memoize would need a special resolver function and a modified storage, because by default it handles only the first argument).

[tleunen]

This will require a new version for the eslint plugin too. I will see if I can move the eslint plugin in this repo too. It will be easier to maintain.