fix(compartment-mapper): remove the file: protocol from __filename an…

[naugtur]

…d __dirname for cjs compatibility

This makes browserify work.

[kriskowal]

This isn’t quite sufficient, accounting for Windows. The readPowers threaded into all of the Compartment Mapper API’s capture url (url.filePathToURL, url.pathToFileURL) so it can convert between URL space and path space.

[naugtur]

Took me a moment to understand why it won't work. The examples here were helpful: https://nodejs.org/api/url.html#urlfileurltopathurl (how dreadful)

When importLocation is used, read powers don't enter compartment-mapper - at least the type signature is defined as just the read function. And I've been using it that way in most places too.
Even if I knew how to drill the reference to fileURLToPath (which is what I need) down to parse-cjs.js, I can't reliably assume I'm going to get it in the first place. That means I need a fallback implementation.
Whichever implementation I use as a fallback, without adding a dependency on node's url, I'll allow for the readPowers being just a function use pattern to continue working fine everywhere that's not Windows.

While I'm personally happy to do so, I think that was not the point here.

I'd need to pass readPowers to makeImportHookMaker and then the set of tools necessary to implement url->path and stuff like require.resolve would need to be the new last argument to parse since parsers don't have any initialization/instantiation and even if they did, readPowers are not available at the time they'd be initialized anyway.

[kriskowal]

I think a reasonable fallback position is to provide __dirname === undefined if the compartment mapper doesn’t receive the canonicalize property on ReadPowers. You may have to change the signature to ReadFn | ReadPowers to thread it, but there are examples where I’ve already done this. There’s a utility function for normalizing that type laying around somewhere.

[naugtur]

readpowers is { read, canonical }.
canonical is not helpful for providing a file path on windows. I will expose existing path2url and url2path functions.

[naugtur]

@kriskowal this should now be ready. One controversy I can see is removing the trailing slash with regex - couldn't find a function that'd do that for me.

[kriskowal]

@kriskowal this should now be ready. One controversy I can see is removing the trailing slash with regex - couldn't find a function that'd do that for me.

This calls for an obscure reference. Barclay says:

const truncateIfFinalSlash = path => {
  if (path.lastIndexOf('/', url.length - 1) === -1) return path;
  return path.slice(0, -1);
};

[naugtur]

@kriskowal

const truncateIfFinalSlash = path => {
  if (path.lastIndexOf('/', url.length - 1) === -1) return path;
  return path.slice(0, -1);
};

I prefer substring with negative values, which I could use too, but the thing is, you insisted on supporting both / and \ which seemed most readable with a tiny regex.
Want me to switch to string manipulation?

[kriskowal]

indexOf('file://') !== -1 has the virtue of not allocating, but I don’t feel strongly.

[naugtur]

appreciated