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(schemas): extend schema and split builders #18

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

kaltepeter
Copy link
Collaborator

@kaltepeter kaltepeter commented Nov 1, 2020

fixes: #9

  • split builder by conversionType
  • adjust schema to support all types

I had originally intended to create a separate schema for each type but that introduced a few challenges. The builders are able to stay isolated easy enough.

Schema options/challenges:

Things to consider:

  • I removed the required fields from base options that have defaults assigned to avoid having to define all base options that have a sensible default. e.g. If this field is in required properties of the schema the user has to still add to every builder config in angular.json
 "srcFiles": {
          "type": "array",
          "description": "Input files matching the given filename pattern.",
          "items": [
            {
              "type": "string"
            }
          ],
          "default": "['*.svg']"
        },

This approach can be take further or require the builder config to provide all required fields and make it easier with #15 generating the default config. @kreuzerk I stopped here as a good point to get thoughts. Maybe issue #15 is a better place to decide.

export default createBuilder<Options & JsonObject>((options: Options, context: BuilderContext) => {
return new Promise<BuilderOutput>(async (resolve, reject) => {
try {
if (options.conversionType !== ConversionType.CONSTANTS) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I remove the check for options.conversionType because schema now requires it and it won't get to the error.
I added a check to match builder with the options.conversionType and removed extra conditionals to simplify.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect - that's also how I imagined it.

Copy link
Member

Choose a reason for hiding this comment

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

With the comment above I think that we can also remove this check. Basically, we would remove the conversionType from the options in this project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gave this a shot and it raises a question. It's trivial to remove the validation bit in the index.ts files. The side effect is I can't remove the requirement from schema.json because that drives the conditional logic and schema checks run before any typescript checks, otherwise I could just for the correct value on the typescript side.

Removing validation would be ok, maybe, because in an ideal world the ng-add setup the builder correctly and it should work. Other fields would also likely fail if the user mismatched them, just not sure they would know why.

Thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

I see. Very good point. I didn't think about this. In this case, I would leave it as it is. Maybe it's even better because then the options are the same as in svg-to-ts

@kaltepeter kaltepeter requested a review from nivekcode November 1, 2020 17:59
Copy link
Member

@nivekcode nivekcode left a comment

Choose a reason for hiding this comment

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

@kaltepeter very very nice, well done 👍 The only thing I would still do before merging is to remove the conversionOption from the schema and the README. What do you think?

After we merged this, I think we are ready to release v1.0.0. The ng-add can then be another feature that follows later, what do you think?

"generate-icons": {
"builder": "@angular-extensions/svg-icons-builder:svg-icons-files-builder",
"options": {
"conversionType": "files",
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to specify the conversionType? I think this is already specified with the builder.

export default createBuilder<Options & JsonObject>((options: Options, context: BuilderContext) => {
return new Promise<BuilderOutput>(async (resolve, reject) => {
try {
if (options.conversionType !== ConversionType.CONSTANTS) {
Copy link
Member

Choose a reason for hiding this comment

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

With the comment above I think that we can also remove this check. Basically, we would remove the conversionType from the options in this project.

svg-icons-builder/constants/index.ts Show resolved Hide resolved
},
"svgoConfig": {
{
"if": {
Copy link
Member

Choose a reason for hiding this comment

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

Very cool - I didn't know that you can use if then statements in schema.json

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't either! I was trying to re-use fields and found this. The nice tool online to test schemas does not yet support this but working in small bits worked.

@@ -0,0 +1,15 @@
import { CommonConversionOptions } from 'svg-to-ts';
import { Delimiter } from 'svg-to-ts/src/lib/generators/code-snippet-generators';
Copy link
Member

Choose a reason for hiding this comment

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

This is also something we should export from svg-to-ts. I will do this and then adjust later once a new version from svg-to-ts is deployed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good!

@nivekcode nivekcode merged commit f67f917 into angular-extensions:main Nov 3, 2020
@nivekcode
Copy link
Member

🎉 This PR is included in version 1.0.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
Development

Successfully merging this pull request may close these issues.

Extend schema.json
2 participants