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

added useLegacyEnums option #144

Closed
wants to merge 5 commits into from
Closed

Conversation

mlankamp
Copy link
Contributor

@mlankamp mlankamp commented Mar 26, 2024

Enables an --useLegacyEnums options as requested in #143

@mlankamp mlankamp changed the title added useLagacyEnums option added useLegacyEnums option Mar 26, 2024
Copy link
Member

@mrlubos mrlubos left a comment

Choose a reason for hiding this comment

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

Are you able to create a changeset entry for the next release?

@mlankamp
Copy link
Contributor Author

Yes, trying to figure out why the test fail

@jordanshatford
Copy link
Collaborator

Yes, trying to figure out why the test fail

Probably the snapshots for test need to be updated with npm run test:update. I am ok adding this in, but is there a reason you can't use --enums and update your uses to use those. They work pretty much the same

@mlankamp
Copy link
Contributor Author

mlankamp commented Mar 26, 2024

They almost work the same, except the new enum name has Enum as a postfix, this would mean that we have to update our 100+ enums in over 2000 vue-files. This option would allow us to upgrade to this package first, and the do the useOptions / Enums upgrade later

src/templates/partials/exportEnum.hbs Outdated Show resolved Hide resolved
src/templates/partials/exportEnum.hbs Outdated Show resolved Hide resolved
@jordanshatford
Copy link
Collaborator

They almost work the same, except the new enum name has Enum as a postfix, this would mean that we have to update our 100+ enums in over 2000 vue-files. This option would allow us to upgrade to this package first, and the do the useOptions / Enums upgrade later

Ok, I am ok with adding this back (after comments are fixed and tests updated/minor changeset added). But it would be under the assumption that it would be removed again at some point (likely when we start migrating away from handlebars)

@mlankamp
Copy link
Contributor Author

likely when we start migrating away from handlebars

What do you want to start using?

@jordanshatford
Copy link
Collaborator

likely when we start migrating away from handlebars

What do you want to start using?

Maybe the typescript compiler api to generate the files. Also the snapshots should not differ with this change (the reason they do is the whitespace added to the templates)

@jordanshatford
Copy link
Collaborator

@mlankamp Once you have fixed the changes and added a changeset, I will merge this and create a release for you to use. Thanks for the contribution

@mlankamp
Copy link
Contributor Author

What do you mean with changeset?

@jordanshatford
Copy link
Collaborator

jordanshatford commented Mar 26, 2024

What do you mean with changeset?

Run npm run changeset and it will take you through a prompt. make it a minor and Description like "Add useLegacyEnums options to generate TypeScript enums" Make sure to run npm run test:update to to update all snapshots

export type {{{name}}} = {{{enumUnionType enum}}};
{{/if}}

{{/if}}
{{#if @root.$config.enums}}
Copy link
Collaborator

@jordanshatford jordanshatford Mar 26, 2024

Choose a reason for hiding this comment

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

Add back one newline above this (below {{/if}}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mlankamp Please add back one newline, run npm run test:update and then add the changeset as mentioned above, I cannot modify this PR. Then I will merge and release

@jordanshatford
Copy link
Collaborator

Completed in #147

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.

3 participants