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

Generics: Use %-encoded escaped names to prevent clashes #475

Closed
wants to merge 1 commit into from

Conversation

WoH
Copy link
Collaborator

@WoH WoH commented Sep 17, 2019

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
  • This PR is associated with an existing issue? Improve Generics Naming #470

Closing issues

Closes #470

If this is a new feature submission:

  • Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach

This changes the names of the generated schemas and therefore may be a breaking change if you rely on the names of the schemas (if you do, please tell me why)

Test plan

I updated existing names. By additionally encoding potentially unescaped by illegal characters, it should be impossible to generate illegal names (although they will be harder to decipher)

I believe we could hold off on this for 3.0 and merge it together with type alias changes I'm currently working on.

@WoH
Copy link
Collaborator Author

WoH commented Sep 17, 2019

A note on contextualized names

The node name TS returns is not always unique and can't be used on itself to determine equality:

interface Generic<T> {
  t: MyInterface<T>
}

let a: Generic<string>
let b: Generic<number>

Will both return the name MyInterface<T> when we parse property t on Generic and clash when we add MyInterface_T_ to the referenceMap / Schema. That's why I replace T with the name from the context.

@WoH WoH marked this pull request as ready for review September 17, 2019 16:47
@WoH
Copy link
Collaborator Author

WoH commented Sep 21, 2019

@dgreene1 would you be comfortable reviewing this?

@dgreene1
Copy link
Collaborator

@WoH if I recall, this is the PR that will need to be a 3.0 release right?

If so, I’m hoping to close some of the minor features first so they can get out to the majority user. Maybe that’s unnecessary though?

@WoH
Copy link
Collaborator Author

WoH commented Sep 21, 2019

I'd personally prefer to merge this in a 3.x release alongside type aliases and some other things concerning type resolution.
Since this is a building block I still would like to make sure this is fine with you or I'm depending on something that's not going to make it which would be awkward.
So review yes, merge maybe? I'm not in a position to decide how tsoa should version itself / when to cut major releases 😉 But I still wanted to point out potential concerns you might have.

@WoH
Copy link
Collaborator Author

WoH commented Sep 25, 2019

@dgreene1 I'd like to build on top of this, should I just keep pushing on top so you can filter commits and review or is should I open more PRs? Does it make sense to target a new branch for the whole thing?

@dgreene1
Copy link
Collaborator

For now it probably makes sense to keep adding to this PR. Unless the new code is for a completely different feature, then it’s okay to make a new PR that depends on this one.

But I have a note in my ToDo app to get back to this so it doesn’t have to languish out here.

Meanwhile I’m investigating how branches and tags work when you publish to NPM. It seems to be a bit of a headache. So my current plan is to just close and publish all of the small items that are in review and then head to v3 as latest.

@WoH WoH mentioned this pull request Sep 25, 2019
10 tasks
@WoH WoH mentioned this pull request Oct 3, 2019
19 tasks
@WoH WoH closed this Oct 3, 2019
@dgreene1
Copy link
Collaborator

dgreene1 commented Oct 3, 2019

@WoH did you mean to close this? If so, why? (Is it because the other PR includes all of the other changes as it’s base?) Btw, I emailed Luke to make you a contributor (which is the only level available for non organizations).

@WoH
Copy link
Collaborator Author

WoH commented Oct 3, 2019

Yes, I closed both PRs and opened the typealias PR based on version 2.5.5.
Since that new PR includes the nameing I felt like it'd be pretty useless to cherry pick the reworked naming commit onto 2.5.5 and open a second new PR.

@dgreene1
Copy link
Collaborator

dgreene1 commented Oct 3, 2019

Okay cool. I just wanted to make sure I didn’t mess you up with that big change.

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.

Improve Generics Naming
2 participants