Refactor

[texastoland]

Proposed changes

1. Abstracts Proxy boilerplate
2. Combines Integers using default startIndex
3. Reduces runtime complexity using Map
4. Tweaks naming for unqualified import
5. Adopts EcmaScript module syntax
6. Adds JSDoc annotations to generate .d.ts
7. Formatted with Prettier

Remaining changes if accepted

• Add Prettier config and script (f9130c3)
• Add tsconfig.json (d04cde5)
• Generate *.d.ts from JSDoc (also d04cde5)
• Deprecate previous names (65fdac1)
• Update tests to use EcmaScript modules (1289066)
• Add ActionCreators in extras module (4e32776)
• Add apply trap (originally 7b7df53)
• Add more tests
• Add colors in extras module
• Update changelog

Followup PR

• Update readme
• Add custom use cases in examples section

[chasefleming]

@texastoland This is awesome! I definitely would love to merge this and the remaining changes you mentioned. One note is I think it would be nice to still keep a way of doing integers without having to execute a function like the other styles -- unless someone wants explicitly set a start index -- in which case I don't see a way around it there. Thanks for adding these updates! Excited to see the rest 😀

[texastoland]

Great thanks! I'll finish tomorrow then. What do you think about:

const Integers = Counter()

I realized it's flexible enough for Redux actions too:

// partially copied from the official implementation
const ActionCreators = (namespace = "") =>
  MemoEnumOf((name) => {
    const type = namespace ? `${namespace}/${name}` : name
    const actionCreator = (payload, { meta, error = false }) => ({
      type,
      payload,
      meta,
      error,
    })
    actionCreator.type = type
    actionCreator.toString = () => type
    actionCreator.match = (action) => type === action.type
    return actionCreator
  })

Usage:

const { increment } = ActionCreators("counter")
// somewhere else

dispatch(increment(step))
// dispatch({ type: "counter/increment", payload: step })

// or simpler yet
const { decrement } = ActionCreators()

dispatch(decrement())
// dispatch({ type: "decrement" })

[chasefleming]

@texastoland Oh wow that's a really cool use case! Saves a lot of lines of code. What do you think about making an examples folder where we put that? Then we could link it on the readme and any other use cases that pop up?

By the way, regarding the naming changes I'm not strongly tied to the original so I'm cool with updating them if you think they are better, but we should probably keep the existing functionality by referencing the new and add a deprecation warning with console.warn until they are removed in a later version.

[chasefleming]

Also, just wanted to say thanks again for your thoughtfulness on these @texastoland ...these are really creative ideas!

[chasefleming]

Hi @texastoland are you still working on the other changes you mentioned?

[texastoland]

Yep finally pulled it down last night and wrapping up!

[texastoland]

The following may not work as expected:

const Integers = Counter()

const { Zero, One } = Integers // => 0, 1
const { ZERO, ONE } = Integers // => 2, 3

Still worth it?

[chasefleming]

@texastoland I think that same issue exists currently if you use the reference instead of instantiating a new Integers(). We can create an issue to address that in a later version.

[texastoland]

I was replying and realized just renaming them:

const Integers /* doesn't imply which ones */ = Counter() /* state is intrinsic */

... resolves the issue from my perspective.

[texastoland]

I got stuck on ezolenko/rollup-plugin-typescript2#304 but rebasing and finishing now 🏁

[texastoland]

@chasefleming I'm ready for feedback before testing and documentation. The checklist in the description links to individual commits.

1 weird thing I noticed is Integers doesn't need a Map unless you expect the same values from the same keys every time. That seems reasonable so I made that behavior dead simple and applied it to Symbols too.

JSDoc types makes the code look more complex than it is. I'll show an example changed to TypeScript in case you're open to it. It already generates typings for TypeScript regardless.

With JSDoc (304 characters/11 lines):

/**
 * @template T
 * @param {(name: string) => T} mapper always returns the same value for the same name
 */
export const MemoEnumOf = (mapper) =>
  EnumOf(
    (name, map) =>
      map.get(name) ??
      /** @type {!T} */ (map.set(name, mapper(name)).get(name)),
    /** @type {Map<string, T>} */ (new Map())
  )

With TypeScript (249 characters/8 lines):

/**
 * @param mapper always returns the same value for the same name
 */
export const MemoEnumOf = <T>(mapper: (name: string) => T) =>
  EnumOf(
    (name, map) => map.get(name) ?? map.set(name, mapper(name)).get(name)!,
    new Map() as Map<string, T>
  )

Just JS (although the types already helped catch my own mistakes):

/**
 * @param mapper always returns the same value for the same name
 */
export const MemoEnumOf = (mapper) =>
  EnumOf(
    (name, map) => map.get(name) ?? map.set(name, mapper(name)).get(name),
    new Map()
  )

PS more than happy to explain anything odd!

[chasefleming]

Thanks @texastoland. Will review as soon as I can. And re TypeScript, this library is purposefully not in TypeScript since enums are a feature there. Feels wrong to write it in TS 😊 It's probably unlikely people would use this if they are using TypeScript (Although, I'm curious to hear a counter to that with the new use cases you thought up. I think if we think up more use cases that could totally change.). But if it is the case that only JS codebases might use this library then the JSDocs are very nice for intellisense, but types files may not even be 100% necessary and just JS could be fine. Or could you just define variables for the arguments with types and then pass them in to make it look cleaner?

[texastoland]

if it is the case that only JS codebases might use this library then the JSDocs are very nice for intellisense

I use TypeScript (and ReScript etc.) because it catches my own errors including several times during this PR. In that sense using types affects writing code more than using it. In this case typings don't help autocomplete in JavaScript since the enum destructures to basically anything (there's nothing to autocomplete).

not in TypeScript since enums are a feature there

I'm surprised enum (as well as namespace) was never officially deprecated. It's gently discouraged in the official handbook and unsupported by Babel (and SWC etc.). The reasons are:

1. It relies on extra code at runtime:

enum Ordinals { A = 1, B, C, D }

// effectively generates the corresponding object

const Ordinals = {
  A:  1,  B:  2,  C:  3,  D:  4,
  1: "A", 2: "B", 3: "C", 4: "D",
}

// COMPARED TO

const { A, B, C, D } = Counter(1)

2. enum (unlike Flow) doesn't automatically generate strings:

enum Seasons {
  Summer = "Summer",
  Autumn = "Autumn",
  Winter = "Winter",
  Spring = "Spring",
}

// not better than

const Seasons = {
  Summer: "Summer",
  Autumn: "Autumn",
  Winter: "Winter",
  Spring: "Spring",
} as const

// COMPARED TO

const { Summer, Autumn, Winter, Spring } = Strings

3. Unions entirely replace enum for strings anyway:

type seasons = "Summer" | "Autumn" | "Winter" | "Spring"

function getSeasonalImage(season: seasons) {}

// type checks
getSeasonalImage("Summer")
// static error
getSeasonalImage("Autum")

// COMPARED TO

const { Summer, Autumn, Winter, Spring } = Strings

getSeasonalImage(Summer)
// runtime error without TypeScript (needs a unit test)
getSeasonalImage(Autum)

a counter to that with the new use cases you thought up

1. Counter is the only API that overlaps enum and still necessary to compile without tsc (including every bundler).
2. Strings is still a relevant style choice compared to string literal unions.
3. Anything more creative (like LowerCased and ActionCreators) is beyond the scope of enum.

could you just define variables for the arguments with types

Technically I could write the .d.tss by hand but would lose type checking internally.

to make it look cleaner

In retrospect it doesn't look bad to me. I added line and character counts to previous #1 (comment) to illustrate the difference in an intentionally extreme example. I'll follow your lead regardless ✌🏼

[chasefleming]

@texastoland Very interesting! I'm sold on use cases in TS. And yeah I don't think it actually looks that bad in JSDoc. This PR looks great. Excited to get this merged in. Awesome contribution. I see a few todo items still listed...are those still coming?

[texastoland]

Yeah I just wanted to make sure you're happy with the code before I finish stuff depending on it!

[chasefleming]

Yeah looks awesome @texastoland ...very happy! Also, definitely think adding that deprecation in the examples section would be nice as well so that we have more than one other use case. Very excited for this merge!

[texastoland]

Documenting an issue I noticed in my previous example:

type seasons = "Summer" | "Autumn" | "Winter" | "Spring"

function getSeasonalImage(season: seasons) {}

// ELSEWHERE

const { Summer, Autumn, Winter, Spring } = Strings

// does NOOOT type check
getSeasonalImage(Summer)
// static error
getSeasonalImage(Autum)

I expected the type of Summer to be the string literal type "Summer". It doesn't work because all the functions are declared using string for names. I tried changing to a generic type TKey extends string everywhere. That basically means:

Each time the function is called it can be a different type but it has to be more specific than string (like "Summer").

That didn't work either though:

Only functions can leave types generic (as opposed to concrete like string) but we're using a proxy (object). Moreover destructuring must happen after any function call. TypeScript never looks "forward" for type information. Therefore nothing more could be inferred for a function either.

In conclusion TypeScript will still need a type annotation for Strings (as opposed to inferring it automatically):

const { Summer, Autum, Winter, Spring } = Strings as Strings<seasons>
//              ^^^^^
//              static error

// type checks
getSeasonalImage(Summer)
// static error
getSeasonalImage(Autum)

[chasefleming]

@texastoland, yeah I see a lot of open GitHub issues around using proxies in TypeScript (specifically around inferring types). Looks like they're playing catch up. Could have another section as well for TypeScript tips with the library linked off of the main readme.

[chasefleming]

By the way @texastoland , I added a CHANGELOG file if you could add a description of the updates to that please.

[mtsknn]

An alternative approach:

const Seasons = Strings<'Summer' | 'Autumn' | 'Winter' | 'Spring'>()

function getSeasonalImage(season: keyof typeof Seasons) {}

getSeasonalImage(Seasons.Autumn) // ok
getSeasonalImage(Seasons.Autum) // error, typo

getSeasonalImage('Autumn') // ok
getSeasonalImage('Autum') // error, typo

// can also destructure but maybe not necessary:
const { Summer, Autumn, Winter, Spring, Foo } = Seasons
//                                      ^^^ error

getSeasonalImage(Autumn) // ok
getSeasonalImage(Autum) // error, typo

TS Playground

But this would probably be useful only for TS users (though JS users could use destructuring). And numeric enums could have wrong order without destructuring (example in the TS Playground).

And eh 🤦‍♂️ I just realized that this is probably unnecessary because as @texastoland said:

Unions entirely replace enum for strings anyway

But whatever, I had fun thinking about this. 🙃

[texastoland]

@mtsknn FWIW I added this:

function getSeasonalImage(season: "summer" | "autumn" | "winter" | "spring") {}

// ELSEWHERE

const { Summer, Autum, Winter, Spring } = Lowercased as Lowercased<
//              ^^^^^
//              static error
  "Summer" | "Autumn" | "Winter" | "Spring"
>

// type checks
getSeasonalImage(Summer)
// static error
getSeasonalImage(Autum)

Or could you just define variables for the arguments with types and then pass them in to make it look cleaner?

@chasefleming Moved them to a separate types.ts in 3460753.

I added a CHANGELOG file if you could add a description of the updates to that please.

Added a todo 🚀

[chasefleming]

@texastoland Cool, hope you don't mind I added "Add deprecated example in other use cases section" as well to the todos.

[texastoland]

@chasefleming Expanded from your review thread:

Does this need to be exposed?

If deprecated.js isn't exposed it's a breaking change. In that case no need to have it?

we should probably keep the existing functionality by referencing the new and add a deprecation warning with console.warn until they are removed in a later version.

Or did I misunderstand?

In terms of other use cases, I was thinking we'd more just make a markdown file for each example use case we think up with a few words about them and then link them off of the readme. (e.g. Making a deprecator with enumOf)

I'm down for that. Or just include a heading in the readme? Can we make a separate docs PR together?

I added the Redux function in an extras module that isn't exported by default. There were several popular npm packages prior to the official createAction. I want to use it as an example too but without unnecessary implementation details.

[chasefleming]

Oh sorry @texastoland you're totally right. I was thinking not to expose the actual deprecated function but it's hidden inside of the deprecated file and just the old methods are exposed. Please ignore my previous comment. Long day :)

And the extras stuff is great. Yeah let's do any docs stuff separate. You've done a lot! Let's get this in :)

[texastoland]

Quick design question I was reminded of by @rauschma's article: when should these be memoized? It boils down to how are they intended to be used?

Conventional enums don't need memoization:

// seasons.js
export const { Summer, Autumn, Winter, Spring } = Symbols

// another module
import { Summer, Autumn, Winter, Spring } from "seasons.js"

// yet another module
const { Summer, Autumn, Winnie, Brooke } = Symbols
// Summer1 !== Summer2

Whereas proxy-fied enums don't necessarily need exports:

// import { Summer, Autumn, Winter, Spring } from "seasons.js"
// ... is technically more typing than ...
const { Summer, Autumn, Winter, Spring } = Symbols

// another module
const { Summer, Autumn, Winnie, Brooke } = Symbols
// with the notable caveat Summer1 === Summer2

The first option helps prevent typos whereas the second just saves typing quotes (and calling Symbol.for). The type would also need imported in the second case regardless as concluded in #1 (comment).

An API solution might be to provide either option depending whether it's used as a function or not:

const { Summer, Autumn, Winter, Spring } = Symbols

// another module
const { Summer, Autumn, Winter, Spring } = Symbols
// where Summer1 === Summer2

// yet another module
const { Summer, Autumn, Winnie, Brooke } = Symbols()
// Summer2 !== Summer3

The intuition would be that the function version acts provides a distinct "scope" similar to functions having a distinct this.

Equivalently for numbers:

const { A, B, C, D } = Integers // 0, 1, 2, 3

// another module
const { A, B, C, D } = Integers // 4, 5, 6, 7
// where A1 === A2

// no need for Counter anymore
const { A, B, E, F } = Integers(1) // 1, 2, 3, 4
// A2 !== A3
// B2 !== A3

[chasefleming]

@texastoland Yeah it seems like there are two use cases we should be intentional about and I like having the option for both. The colors and mood example from the article is a good one:

// EXAMPLE 1

// colors
const {red, green, blue} = Strings

// mood
const {happy, blue} = Strings

blue === blue // should be false

In that case, blue should not be the same. Symbol is good here. But there is a use case where someone may have a string from a JSON response they need to check against the enum (for example):

// EXAMPLE 2

const { v1 } = Strings

v1 === "v1" // we want to be true

So correct me if I'm wrong but what you are proposing is to make "Example 1" use Strings() instead of Strings as an indication to use Symbol and scope it.

Which I think is actually better than TypeScript enums, because if I'm not mistaken the two "Blues" in the following example evaluate to true:

enum mood {
  Blue,
  Happy
}

enum colors {
  Blue,
  Yellow
}

mood.Blue === colors.Blue // should not be true but is in TypeScript

I'm very into the optional scoping, but technically, how would you go about making that scoping options though? You'd have to return an object or an IIFE then how would you invoke another function on top of that? Not sure you could curry an IIFE since it doesn't return a function(or an optional one at that).

But I also think it may cause confusion to a developer. It's a pretty subtle difference and I think being more explicit is probably prone to less issues for developers. So something like this would maybe be clearer.

const { blue, happy } = Strings({ unique: true })

// or

const { blue, happy } = Strings().unique()

// or 

const { blue, happy } = Strings().isUnique()

But that loses the simplicity in my opinion and starts to get pretty ugly and lengthy...which is one of the reasons I went with the original design. Maybe you have a suggestions that is more explicit design but simpler? Although if it's a rarer case that you'd want it scoped it's not terrible to have the additional method and my vote would be for .unique(). Maybe StringsLower would also be replaced Strings().lower() in this case? Then could chain unique if you wanted.

(If we do think up other options though a case can be made for other options then a function approach would probably make more sense anyway)

[texastoland]

Moving tests and PR checks todos to separate PRs as suggested on Discord.

[chasefleming]

@texastoland let's do the colors item as a separate PR as well so we can get this in. Should probably have tests for the new method namings at least, but should be able to copy those easily from the existing tests. Think changelog should be in this one as well under "Unreleased"

[texastoland]

Tests passing but 1 small typing error to fix 🐛

[chasefleming]

nice touch!

[texastoland]

The diff is confusing even for me. We have the conversation here for context. Opening sequential PRs for more granular changes ASAP!