619888 - Add a JavaScript dictionary which can support arbitrary keys

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: patch, v1

See the URL for why this is needed, etc.

The dict.js itself is a copy of what's in [1]. The tests seem to cover all that we guarantee and none that we do not.

The ES5 spec doesn't guarantee anything about mutation during iteration, so we don't either.

[1] https://github.com/sid0/jsdict/tree/core/

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 498291 [details] [diff] [review]
patch, v1

Fantastic work.  Thank you so much for looking into this problem space so comprehensively!

Although we can probably do a performance pass later if the profiler says so, it's not clear there's really any benefit to having "items" live inside _state since we never appear to clobber it.  (For example, unless the method JIT is inlining convert/unconvert, it might not be crazy to inline those in some cases.)

It's not clear to me why you are making null be the default value for missing values.  It seems like letting it remain undefined is slightly more expressive without making the caller's life harder since they can just do "== null" if they don't want the nuance.  I mainly mention this because I expect it would make porting existing code slightly easier since the semantics are then closer to a straight-up JS object.

In the constructor, you don't need to use hasOwnProperty on aInitial because Iterator's semantics are that it doesn't climb the prototype chain or see __proto__.  (I'm not sure where that's actually hardcore defined but when it regressed and I complained they put it back that way, so I figure we're safe :)

I have to admit to being slightly concerned about calling this dict.js and having it be at the root of modules.  Unless we are going to try and get this landed in mozilla-central too, maybe we should put it under a directory or namespace it ever so slightly?

r=asuth with null allowed to just use undefined as-is, although I have been known to listen to reason.

Comment 2

[Rain [:rain1] (inactive)]

(In reply to comment #1)
> Although we can probably do a performance pass later if the profiler says so,
> it's not clear there's really any benefit to having "items" live inside _state
> since we never appear to clobber it.  (For example, unless the method JIT is
> inlining convert/unconvert, it might not be crazy to inline those in some
> cases.)

We don't even have the method JIT on in chrome yet, so hm.

I did have items separately at first, but once I introduced count (and needed to wrap it to avoid issues with Object.freeze) I thought it was just cleaner to have it this way.

> 
> It's not clear to me why you are making null be the default value for missing
> values.  It seems like letting it remain undefined is slightly more expressive
> without making the caller's life harder since they can just do "== null" if
> they don't want the nuance.  I mainly mention this because I expect it would
> make porting existing code slightly easier since the semantics are then closer
> to a straight-up JS object.

OK, fair enough. I think I was looking at the python dict while deciding what should happen here, but since python doesn't have separate undefined and null, this makes more sense.

> 
> In the constructor, you don't need to use hasOwnProperty on aInitial because
> Iterator's semantics are that it doesn't climb the prototype chain or see
> __proto__.  (I'm not sure where that's actually hardcore defined but when it
> regressed and I complained they put it back that way, so I figure we're safe :)

OK.

> 
> I have to admit to being slightly concerned about calling this dict.js and
> having it be at the root of modules.  Unless we are going to try and get this
> landed in mozilla-central too, maybe we should put it under a directory or
> namespace it ever so slightly?

Sure, what do you suggest? re landing in m-c, we can always stuff it in a separate dir now and move it (+ keep the old copy around as a legacy fallback) if and when it lands in m-c.

Comment 3

[Mark Banner (:standard8)]

I'm generally happy with the idea of this, and it seems like the first use we'll have will be in gloda.

(In reply to comment #2)
> (In reply to comment #1)
> > I have to admit to being slightly concerned about calling this dict.js and
> > having it be at the root of modules.  Unless we are going to try and get this
> > landed in mozilla-central too, maybe we should put it under a directory or
> > namespace it ever so slightly?
> 
> Sure, what do you suggest? re landing in m-c, we can always stuff it in a
> separate dir now and move it (+ keep the old copy around as a legacy fallback)
> if and when it lands in m-c.

We could possibly put it in utils/dictUtils.js as one thought. Is Dict the right thing to call it in this case as well? I guess extensions should be able to work around differences in APIs if necessary.

I don't think we really need to keep a fallback these days - extensions should migrate from one thing to another, although I guess with importing modules we can just import the m-c version into our old file and re-export? This would probably only really work if the APIs are the same.


Generally I'm happy with this patch but I think I should see a new one before giving it an sr+.

Comment 4

[Rain [:rain1] (inactive)]

Attachment: patch v2

OK, I've moved the file to dictUtils.js. We still call it Dict, since if there's an API breakage extensions can import the module into a non-global scope.

Comment 5

[Rain [:rain1] (inactive)]

Landed: https://hg.mozilla.org/comm-central/rev/a5c37e0b35b7

Comment 6

[Rain [:rain1] (inactive)]

Note that this is going to move to m-c soon -- see bug 621204.

Comment 7

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Dict.jsm