Skip to content

feat: user-defined required constructor options #63

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

Merged
merged 12 commits into from
Jul 20, 2021

Conversation

gr2m
Copy link
Owner

@gr2m gr2m commented Jul 17, 2021

closes #60

TODOs

  • create examples/required-options folder with a README, code & tests that implements and tests a user-defined required constructor options
  • adapt npm test to run all code & type tests from both the root folder
  • remove the hard-coded version required option from Base and make it dynamic based on user-defined options instead
  • update tests in /index.d-test.ts that test for the required version option and move them to examples/required-options/index.test-d.ts instead

@gr2m gr2m force-pushed the 60-handle-user-defined-required-options branch from 43a90b9 to 61036c4 Compare July 17, 2021 21:31
@gr2m
Copy link
Owner Author

gr2m commented Jul 17, 2021

@JoshuaKGoldberg I think I'm all set from my side, I added the example repository with some code and tests. It's all up for discussion if something is unclear or if you want to suggest a simpler/better implementation

Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>
@gr2m
Copy link
Owner Author

gr2m commented Jul 18, 2021

I forgot about moving the tests for the required option into the new example folder, I added a TODO

  • update tests in /index.d-test.ts that test for the required version option and move them to examples/required-options/index.test-d.ts instead

@gr2m gr2m self-assigned this Jul 19, 2021
@gr2m
Copy link
Owner Author

gr2m commented Jul 19, 2021

@JoshuaKGoldberg are you done from your side? Tests are still failing so I'm not sure if you are still looking into it, happy to review it as is and try to fix the remaining problems

@gr2m
Copy link
Owner Author

gr2m commented Jul 19, 2021

Currently the IntelliSense in VS Code is happy about this code, but tsd fails with

Expected 1 arguments, but got 0

for the last line 👇🏼

const BaseLevelOne = Base.plugin(fooPlugin).defaults({
  defaultOne: "value",
  required: "1.2.3",
});

// Because 'required' is already provided, this needs no argument
new BaseLevelOne();

@JoshuaKGoldberg
Copy link
Collaborator

Ah sorry, I ended up running out of time yesterday. Will tackle more tonight!

@gr2m
Copy link
Owner Author

gr2m commented Jul 19, 2021

no problem at all, I was just checking in

@gr2m gr2m removed their assignment Jul 19, 2021
Comment on lines +41 to +45
[K in keyof Obj]: {} extends Pick<Obj, K> ? undefined : K;
}[keyof Obj];

type RequiredIfRemaining<PredefinedOptions, NowProvided> =
NonOptionalKeys<RemainingRequirements<PredefinedOptions>> extends undefined
Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg Jul 20, 2021

Choose a reason for hiding this comment

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

This was an odd little issue. tsd was giving different issues the different errors than VS Code's intellisense. My best guess for a root cause would be something about different compiler options in tsd' defaults vs. VS Code's when there isn't a tsconfig.json.

I debugged by adding a debugKeys: NonOptionalKeys<RemainingRequirements<PredefinedOptions>>; in the class returned by ConstructorRequiringVersion's new: 32744ac

...which showed itself to be type never in VS Code but type undefined in tsd. 💡

expectType<{ intentionallyFailAlways: true }>(new BaseLevelOne().debugKeys)

I switched from never to undefined to make sure both variants of possible compiler options work.

Copy link
Owner Author

Choose a reason for hiding this comment

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

My best guess for a root cause would be something about different compiler options in tsd' defaults

just fyi, here is tsd's default config:

{
	"strict": true,
	"jsx": "react",
	"target": "es2017",
	"lib": ["es2017"],
	"module": "commonjs",
	// The following option is set and is not overridable:
	"moduleResolution": "node"
}

https://github.com/SamVerschueren/tsd#custom-typescript-config

@JoshuaKGoldberg
Copy link
Collaborator

@gr2m now it's ready 👍

@gr2m gr2m self-assigned this Jul 20, 2021
Copy link
Owner Author

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

awesome work, thanks for making it work, and for the insights on how you debugged the different behavior between tsd and VS Code's IntelliSense

@gr2m gr2m merged commit d5fd26f into main Jul 20, 2021
@gr2m gr2m deleted the 60-handle-user-defined-required-options branch July 20, 2021 16:50
@github-actions
Copy link

🎉 This PR is included in version 3.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants