Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[refactor - typescript] object-transforms #343

Conversation

jfdnc
Copy link
Contributor

@jfdnc jfdnc commented Oct 26, 2021

Refactors object-transforms util file to TypeScript with newly defined Options type for configuration options object, includes a couple of cleanup steps described in the commits.

@jfdnc jfdnc force-pushed the refactor/typescript/object-transforms branch 2 times, most recently from a75ffcf to b4032ae Compare October 26, 2021 18:12
@jfdnc jfdnc changed the title Refactor/typescript/object transforms [refactor - typescript] object-transforms Oct 26, 2021
Copy link
Contributor

@jherdman jherdman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for not getting to this sooner.

addon/utils/object-transforms.ts Outdated Show resolved Hide resolved
addon/utils/object-transforms.ts Outdated Show resolved Hide resolved
addon/utils/object-transforms.ts Outdated Show resolved Hide resolved
addon/utils/object-transforms.ts Outdated Show resolved Hide resolved
@jfdnc jfdnc force-pushed the refactor/typescript/object-transforms branch from b778731 to a50c404 Compare November 13, 2021 02:58
@jfdnc
Copy link
Contributor Author

jfdnc commented Nov 13, 2021

@jherdman Thanks for feedback! No worries about time turnaround, I've had a super busy week myself.

Had to force push here to resolve master conflict in a test (assert.equals(true... -> assert.true(...), review feedback is in most recent two commits.

I'm Interested in your thoughts on using the Options type for the descriptive internal API, but if you feel strongly I'm glad to make it a bit more generic. Thanks!

@jherdman
Copy link
Contributor

I'm Interested in your thoughts on using the Options type for the descriptive internal API, but if you feel strongly I'm glad to make it a bit more generic. Thanks!

I'm down for this naming. I left a suggestion above, but I'm also happy to leave it as is too.

- refactors `object-transforms.js` to `.ts`
  - adds `Options` type of `Record<string,string>` to match expected config options object
  - `compact` and `without` wrap new shared `includeKeys` fn
    - were both doing essentially the same thing, returning new config options with subset of given config options based on some check
  - removes unused `only` fn
    - existed only in utils test
  - minor refactor of `isPresent` for single-line check of emptiness
  - adds descriptive jsdoc headers
current `isPresent` function is name-aliasing `@ember-utils#isPresent`, but serves a different functional purpose. Ember's `isPresent` will return `true` for an object with no keys, but the the util function we're exposing as `isPresent` is checking if the given object has properties. given that this is an Ember addon, we should avoid overloading the concept of "present" in the context of an object's property names.

- renames `object-transforms#isPresent` to `object-transforms#hasOwnProperties`
- updates imports and instances throughout
- collapses import of `@ember/utils#isPresent` in `object-transforms#compact`
- destructure `object-transforms` imports at callsites
- some util imports were relative paths, some absolute, call all as absolute paths
@jfdnc jfdnc force-pushed the refactor/typescript/object-transforms branch from a50c404 to 4993e3a Compare November 13, 2021 22:18
@jfdnc
Copy link
Contributor Author

jfdnc commented Nov 13, 2021

@jherdman Feedback squashed into the relevant commits, and tacked on one more commit for the folder restructuring.

@jfdnc jfdnc requested a review from jherdman November 13, 2021 22:19
Copy link
Contributor

@jherdman jherdman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks!

@jherdman jherdman merged commit e6bfeeb into adopted-ember-addons:master Nov 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants