Integrating and providing Enum Objects

[DarioSoller]

This PR proposes enum like JS objects for most hardcoded string configuration values. As discussed in the related issue #1367 .

A few remarks for the review of this PR:

• Besides the sheer amount of changes, no logic has been changed and it's solely replacing hard coded strings with their respective variable.
• Important consideration is, if and to what extend you want to promote these available enums? Especially regarding the appearance in the JSdoc code snippet examples!? For the sake of keeping these documentation code snippets simple, I can also revert the changes I have done there.
• Import paths for all examples should then be the final import path from the actual style-dictionary npm package. I will update that accordingly, as soon as I know, how you want it to be? Should it directly come from style-dictionary or maybe similar like the types style-dictionary/types something like style-dictionary/enums?
• Where in the project scaffolding are these enums best placed in your opinion? For now all enum JS objects live within one file lib/buildInEnums.js, but I can also make a folder with one file per enum object?
• I stumbled over one questionable file format ('json/nested'), which seems to be used internally, but should probably be not available as a "public" file format configuration option and therefore not part of the enum!? I am talking about https://github.com/amzn/style-dictionary/blob/main/lib/common/formats.js#L1538
• Surprised about the extend of necessary changes in one of the first commits for the logging, commentStyle, commentPosition have split up all subsequent changes in their separate commits. If wanted, I can interactively rebase and split up changes further, or I can also squash things accordingly, depending on how you want to see the changes in commits for the generated changelog later on?
• When is the best time to merge such a big change, without interfering with other open PRs and code changes, in order to minimize potential merge conflicts?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[aws-amplify-us-west-1]

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1375.d16eby4ekpss5y.amplifyapp.com

[jorenbroekema]

• Sounds like a good idea to promote the enums
• style-dictionary/enums sounds good
• lib/enums with separate files formats / transforms, etc. sounds good
• json/nested is public and used by some users for sure so I would add it yeah
• Eventually we'll squash changes yeah, probably into 1 commit, usually 1 commit per changelog/change, sometimes that's many file changes if it means a refactor, that's ok
• Never haha, never a good time, but it's needed nevertheless and there aren't that many active PRs open apart from 2-3, I'll deal with the merge conflicts that happens for other open PRs

Will do a full review in a bit.. or maybe you want to first address the above answers and I review after the changes you do as a result of my answers?

[jorenbroekema]

Just did a quick glance and in general I think we can:

• go for brevity for some of these enum names, so they are less wieldy to use/write for people
• destructure/dedupe repeated elements/enum members

And we'll need to add a small barrel file: lib/enums/index.js that we can point to in our package.json -> exports -> "./enums": "./lib/enums/index.js"

I'll give a more detailed thorough and detailed review eventually when the broader stuff is aligned a bit more

[DarioSoller]

@jorenbroekema I added most of the discussed changes/updates except for the following, which is still a TODO:

• destructure/dedupe repeated elements/enum members
• Updating import paths in the examples to style-dictionary/enums

Maybe you want to wait with a next review, until I have done these last changes, as well?

[DarioSoller]

@jorenbroekema So I think now would be a good moment to make a more in depth review, if you have time. Following points should get a close inspection:

1. Destructering and depuping were only done, where it makes sense. So only at places with more than one occurrence for any of the enum values. This has been a lot of work, cause each file is kind of individual in regard of which enum values are needed. TBH I am not 100% convinced that this is the big advantage, because it all boils down to consistency vs. readability!? Anyhow it's done now and maybe it really was worth an one-time effort.
2. Certain transform values, that are either a specific example or some old legacy predefined transforms, like here or here
3. No changes done in docs/src/content/docs/version-4/migration.md, because these docs only show the migration to v4.0.0 right?
4. I had issues with a few search and replaces during the refactor, because it seems that backticks are also regarded as singleQuotes by Prettier!? Not sure about it, but there are a few string values in backticks, where actually no template string is needed, which I missed in my initial refactorings. I hope to have them all now!?
5. When I do an Style-Dictionary build, I am still not sure what our ideal outcome for the type definition files of these enums should be? Currently it ends up to be for example:

// lib/enums/commentPositions.d.ts
export namespace commentPositions {
    let above: string;
    let inline: string;
}

[jorenbroekema]

I didn't end up getting to this yesterday/today due to other prios but I'll try to get on this Monday!

[jorenbroekema]

Looks good :)

Three things:

• We should have a docs page -> Enums, and refer to it where necessary when we refer to the enums in other places in our docs
• really wanna call fileFormats formats to be honest, just because that's consistent with the name of the hook "formats"
• I think the enums members currently are loosely typed as just "string", in JSDocs it seems like from my experimentation the only way to strictly type the enum members is to cast every member to the exact string value. I was hoping @readonly or @enum tags in JSDocs would do it, especially @readonly I expected to work as enum but TypeScript doesn't agree with me :\ I pushed a commit to your branch just to show an example of how you would do this so that's three parts:
  • adjust the enums to be strict
  • remove typecasts in the JS source files where they are no longer needed
  • adjust the types TS files where needed to essentially use (typeof <enum>)[keyof typeof <enum>] to type the properties as "value of enum"

[DarioSoller]

Further, do you think there is anything for the types/Format.ts that we could use an enum for, in order to be more precise with the typings? Not sure here, because custom formats and custom transforms always have to be possible and I am therefore uncertain if it makes any sense to type it something like:

import { formats } from '../lib/enums/index.js';
type formats = typeof formats;

export interface Format {
  name: string | formats[keyof formats];
  format: FormatFn;
}

[DarioSoller]

And one more thing, which I noticed in the docs of the format-helpers. There are existing different options.formatting string values, which we have not covered yet as an enum?

[DarioSoller]

I am not sure what causes the CI pipeline to fail atm!? Maybe someone can see more of the AWS Amplify logs than me?

[jorenbroekema]

Further, do you think there is anything for the types/Format.ts that we could use an enum for, in order to be more precise with the typings? Not sure here, because custom formats and custom transforms always have to be possible and I am therefore uncertain if it makes any sense to type it something like:

import { formats } from '../lib/enums/index.js';
type formats = typeof formats;

export interface Format {
  name: string | formats[keyof formats];
  format: FormatFn;
}

Yeah that seems like a good idea for a bit better autocomplete if possible.

options.formatting is an object, each property can be any string value, so I think it's okay as is.

I'll take a look at the merge conflicts and the pipeline failure and commit a fix if needed.

[DarioSoller]

@jorenbroekema If you like I can also rebase this branch to the current main and resolve the conflicts that way? Most of it should be in the import paths with the .tsto .js change. Just let me know, what the AWS Amplify logs say about the failing. I think then we could be done with this PR, what do you think?

[jorenbroekema]

I'll take another look this weekend/Monday (probably) and get this PR over the finish line, including rebase etc. 👍🏻

[jorenbroekema]

Alright I just squashed and rebased and fixed all of the stuff I encountered during thorough review, summary of what I adjusted, which can be viewed separately here 1e2e088 (note however that GitHub will eventually garbage collect this orphan commit):

• Some destructuring here and there (formats/transforms file)
• Missing enums that I ran into, e.g. json/asset format
• Add Enums section in docs to the sidebar and adjusted content ever so slightly for accuracy
• Made sure the newly added format uses enums as well (javascript/esm)
• Added a changeset entry for the changelog, minor bump seems appropriate since this is a pretty big feature
• Fixed a couple of test files that failed but didn't cause the runner to exit with an error code, this can happen if it fails to resolve imports in test files for example
• Fixed sd-playground / rollup bundle util to deal with multiple named SD imports, this is a bit complicated so don't worry about it too much, the enums can be used in the playgrounds now :)

To wrap this up we just need a review from @dbanksdesign but from my perspective this PR is now ready :) thanks again for contributing @DarioSoller

[dbanksdesign]

There are some broken links in the docs with this PR:

2024-11-12T11:06:01.436Z [WARNING]: 11:06:01 [ERROR] ✗ Found 4 invalid links in 2 files.
2024-11-12T11:06:01.436Z [INFO]: 11:06:01 ▶ reference/utils/format-helpers/
11:06:01 └─ /reference/enums#commentStyles - invalid hash
11:06:01 ▶ reference/api/
11:06:01 ├─ /reference/enums#logWarningLevels - invalid hash
11:06:01 ├─ /reference/enums#logVerbosityLevels - invalid hash
11:06:01 └─ /reference/enums#logWarningLevels - invalid hash
2024-11-12T11:06:01.639Z [WARNING]: [AstroUserError] Links validation failed.

[DarioSoller]

@dbanksdesign thanks for sharing the Amplify error logs. Yeah, I see these page anchors need to be dasherized. I just pushed a fix. 🤞

[DarioSoller]

@dbanksdesign as it is still failing, could you share the Amplify error logs one more time with me?