Add support for browser field in package.json

[KidkArolis]

Based on the unofficial spec https://gist.github.com/defunctzombie/4339901

This is not complete yet, but I just wanted to get some feedback before I proceed.

If the browser field is a string, I simply map that onto descriptor's main, since that what it actually means. If the browser field is an object, I create a new descriptor.map property which is then used in the normalization hook. This way both loading and requiring modules goes through the mapper

There's one more thing missing which is being able to set modules to false in the map to prevent them from being loaded. I was thinking about creating a rave/blank module and map it to that.

If this sounds ok, I'll proceed with making sure it actually works well (i.e. write tests).

[KidkArolis]

Btw, rave's code is really nice to read through and work with ;)

[unscriptable]

on first glance, this looks awesome. gonna dig in a bit more very soon.

Btw, rave's code is really nice to read through and work with ;)

thanks. trying to do it right. just don't look at rave/auto.js. :) it needs some work atm.

[KidkArolis]

I'm now normalizing the module ids based on the package root. I wasn't sure if I should be doing that in the npm crawler or somewhere after the descriptor has been created. I thought it could be nice to allow unnormalized map on the descriptor and take care of the normalization somewhere later where other normalization is done.

Or more importantly - I'm not sure whether the normalization I'm doing in the npm crawler will always match the normalization done in the lib/createNormalizer - i.e. I'm now pulling in lib/uid and pipeline/normalizeCjs to carry out the normalization, but it seems slightly brittle.

[unscriptable]

it seems slightly brittle.

I agree. When we start handling ES6 npm modules, we'll def have to revisit this code. Tbh, the normalization code needs to be streamlined. There are too many calls to findPackage() in the code path. However, there are more important issues to solve, atm, imho.

[unscriptable]

I wasn't sure if I should be doing that in the npm crawler or somewhere after the descriptor has been created.

I can't find a better place, atm. :)

[KidkArolis]

I agree that normalizeCjs shouldn't know about the uids. It also makes sense to not create uids in the crawler - it makes that way less brittle - now it just does path normalization based of the root of the package.

I was a bit confused about what to call things in the comments.

foo@1.0.0#foo/some/file

Is the whole thing uid? Then what is foo@1.0.0 and what is foo/some/file?

(I will add more tests to all of this)

[unscriptable]

Here's my nomenclature:

foo@1.0.0#foo/some/file is a "uid" or "module uid" foo@1.0.0 is a "package uid" foo/some/file is a module name (ES6 terminology)

[KidkArolis]

At first I wasn't sure about the name createPackageMapper, but now it makes sense - mapper is a more generic moduleName > moduleName mapper, packageMapper is a mapper for use with packages - i.e. it handles mapping module uids.

Also I named the new uid function getModuleName to be more specific than getName.

Waiting for further feedback ;)

[unscriptable]

Then add a few tests for getModuleName() and I think we're done!

[KidkArolis]

The reason I didn't do the sniff in the getModuleName is because I wasn't sure I could reliably detect if the argument to getModuleName is packageUid or module name or module uid. Now it only accepts module uid as the arg so the caller needs to ensure its a module uid and not module name or package uid (just like uid.parse). Otherwise what happens if its called with

getModuleName('foo@1.0.0')
//vs
getModuleName('some/path')

Is it safe to check for @ to decide its a package uid?

— Reply to this email directly or view it on GitHub.

[unscriptable]

parseUid works if you pass it a module uid or module name. I should probably document this better, of course, but check out the pop() on line 17.

ugh. Total test fail on my part: https://github.com/RaveJS/rave/blob/master/test/lib/uid.js#L39

Either we need to allow both uids and names in all of these functions or we need to provide a way to test for uids from within the uid module: function isUid(thing:string): boolean. I like the simplicity of calling code when the functions allow both, but I could probably be convinced otherwise.

[KidkArolis]

Current outputs

parseUid("foo@1.0.0#foo/main/index")
Object {name: "foo/main/index", pkgName: "foo", modulePath: "main/index", pkgUid: "foo@1.0.0"}

parseUid("foo@1.0.0")
{name: "foo@1.0.0", pkgName: "foo@1.0.0", modulePath: "", pkgUid: undefined}

parseUid("foo/main/index")
Object {name: "foo/main/index", pkgName: "foo", modulePath: "main/index", pkgUid: undefined}

but shouldn't parsing package uid be more like..

parseUid("foo@1.0.0")
{name: "", pkgName: "foo@1.0.0", modulePath: "", pkgUid: "foo@1.0.0"}

I guess I'm just confused about what name means in this context?

[KidkArolis]

Sorry, I can't take a closer look at it all right now, I'll get back to you as soon as I review this and have a think.

[unscriptable]

Interesting. I hadn't considered that somebody might pass a package uid instead of a module uid or module name.

I've been using "name" in the ES6 sense, similar to AMD's "id": the canonical name of the module you want to import. "foo/main/index" is a canonical name, for instance. "index" and "./index" are not.

[KidkArolis]

If uid.parse and uid.getModuleName (or in this case, uid.getName might be sufficient) only need to take in module names or module uids, then it shouldn't be a problem to make getName work with both!

[unscriptable]

right.

is there any way i can pick up where you left off? or do you think you'll have some more time soon?

[KidkArolis]

Pushed some updates. Gtg now (well, will be back tomorrow) - feel free to take over.

[unscriptable]

This looks ready to go, @KidkArolis. Gonna merge this later today unless there are objections. :)

[unscriptable]

Very happy to merge this! Thanks @KidkArolis. :metal:

[KidkArolis]

woohoo!