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

feat(functions): export functionOptions #1904

Merged
merged 8 commits into from
Oct 18, 2021
Merged

Conversation

billiegoose
Copy link
Member

Refactoring to make it easier to import the functionOptions schemas from @stoplight/spectral-functions. The function options are currently exposed on each individual function, but since the functions aren't very portable (relying on certain shims and specific bundler features that are not widely supported) that has caused issues for Stoplight developers working on the Spectral UX. By building and exporting a simple JSON object with all the functionOptions we can stay in sync with Spectral rather than maintain separate hard-coded values.

Does this PR introduce a breaking change?

  • Yes
  • No

Screenshots

Here's a (silly) example demonstrating the functionOptions being used in Storybook.

Spectral.Core.Function.Option.Schemas.mov

@P0lip
Copy link
Contributor

P0lip commented Oct 18, 2021

@wmhilton make sure to commit all the files, it seems like you forgot to commit a few and tests fail.

specific bundler features that are not widely supported

ha? package exports is supported by all major bundlers

@billiegoose
Copy link
Member Author

make sure to commit all the files, it seems like you forgot to commit a few and tests fail.

oh thank god. I was like "why the HELL are these linting errors saying it's of type any" I'm still on my first ☕

ha? package exports is supported by all major bundlers

We aren't all on the bleeding edge. Some of us still use Node 12 and Webpack 4!

@billiegoose
Copy link
Member Author

OK it should be good now :D

Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Overall OK.
I'd remove CustomFunctionOptionsSchema as it doesn't seem to bring much of a value.
Do keep in mind that this may break anytime, as Spectral will inevitably become fully ESM, so nested exports may break as they are never considered as a part of the public API.

packages/functions/src/schema/optionSchemas.ts Outdated Show resolved Hide resolved
@@ -73,6 +73,8 @@ type SchemaDefinition = Schema | boolean;

const DEFAULT_OPTIONS_VALIDATOR = (o: unknown): boolean => o === null;

export type CustomFunctionOptionsSchema = Schema | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need this type? Feels somewhat redundant 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed Schema to be exported.

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure how you expect users to write custom functions when the type isn't exported...

@@ -0,0 +1,18 @@
export enum CasingType {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move this back to casing function?

Copy link
Member Author

Choose a reason for hiding this comment

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

problem is it's as a value later, and casing.ts pulls in things from @stoplight/spectral-core, so I can't easily import it in optionSchemas.ts without it blowing up from a small JS file into a monster with dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

The general pattern we've adopted at Stoplight is to put as many pure types into a separate types.ts file so that the types can be re-used without dragging around implementations. I'm not sure why Spectral doesn't do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The general pattern we've adopted at Stoplight is to put as many pure types into a separate types.ts

Never heard of it, to be honest. 😆
We do that in platform-internal, but it's quite project-dependant overall.

Copy link
Contributor

Choose a reason for hiding this comment

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

but yeah, I see now. It's alright.
TBH, I'll probably incorporate json-schema-to-typescript at some point, so we don't need to maintain these option types anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

json-schema-to-typescript

👀 ooooh

Copy link
Contributor

Choose a reason for hiding this comment

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

json-schema-to-typescript - we actually use it already in Spectral for ruleset-migrator. It works pretty well!

@P0lip
Copy link
Contributor

P0lip commented Oct 18, 2021

We aren't all on the bleeding edge. Some of us still use Node 12 and Webpack 4!

FWIW Node.js 12 does support it just fine.
Well, Webpack v4 is becoming barely usable in the year of ESM, so we just gotta try to migrate, as it causes a true pain in the case of some of our packages (not Spectral tho). 😬

@billiegoose
Copy link
Member Author

Do keep in mind that this may break anytime, as Spectral will inevitably become fully ESM, so nested exports may break as they are never considered as a part of the public API.

That's fine. It's still better than maintaining it separately in platform-internal IMO.

@billiegoose
Copy link
Member Author

I'd remove CustomFunctionOptionsSchema as it doesn't seem to bring much of a value.

Currently Schema | null isn't exported. So it only works by type inference of the function arguments.

Would you like me to change

export const optionSchemas: Record<string, CustomFunctionOptionsSchema>

to

export const optionSchemas: Record<string, Parameters<typeof createRulesetFunction>[0]>

:trollface:

@P0lip
Copy link
Contributor

P0lip commented Oct 18, 2021

export const optionSchemas: Record<string, Parameters[0]>

yeah, that's OK.

@billiegoose
Copy link
Member Author

billiegoose commented Oct 18, 2021

lol it ended up being

export const optionSchemas: Record<string, Parameters<typeof createRulesetFunction>[0]['input']>;

but it works


type CustomFunctionOptionsSchema = Parameters<typeof createRulesetFunction>[0]['input'];

export const optionSchemas: Record<string, CustomFunctionOptionsSchema> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

packages/functions/src/schema/index.ts you need this one too

Copy link
Member Author

Choose a reason for hiding this comment

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

oooh I didn't even notice that one ha.

P0lip
P0lip previously approved these changes Oct 18, 2021
@P0lip P0lip added the enhancement New feature or request label Oct 18, 2021
@P0lip P0lip merged commit 2f26ed6 into develop Oct 18, 2021
@P0lip P0lip deleted the feat/functions-schemas.json branch October 18, 2021 18:53
stoplight-bot pushed a commit that referenced this pull request Oct 19, 2021
# [@stoplight/spectral-functions-v1.3.0](https://github.com/stoplightio/spectral/compare/@stoplight/spectral-functions-v1.2.1...@stoplight/spectral-functions-v1.3.0) (2021-10-19)

### Features

* **functions:** export functionOptions ([#1904](#1904)) ([2f26ed6](2f26ed6))
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version @stoplight/spectral-functions-v1.3.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@pamgoodrich
Copy link
Contributor

@wmhilton : Is this something that customers would be interested in or is it something that impacts Stoplight devs only?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants