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

Bugfix/356 #360

Merged
merged 15 commits into from
Jun 9, 2021
Merged

Bugfix/356 #360

merged 15 commits into from
Jun 9, 2021

Conversation

michelengelen
Copy link
Contributor

Bumping typescript to 4.3.2 and @types/node to 15.6.1 and fixing resulting errors.
This is done to fix the issue with (tag.text || "").trim is not a function error message on recent installations of typescript (for example in storybook)

Fixes #356

michelengelen and others added 13 commits June 1, 2021 13:58
@PeachScript
Copy link

👍

PR has file conflict, and what is the plan to release it?

@michelengelen
Copy link
Contributor Author

@PeachScript I would like to send it into the next release... It should be when @pvasek merges this PR!

@PeachScript
Copy link

@pvasek it would be better if you could take the time to review it 😆

@PeachScript
Copy link

@pvasek 👀

@fforres
Copy link
Contributor

fforres commented Jun 7, 2021

This is amazing <3
(It's blocking on of our upgrades also 😄 )

BTW.. Should the peerDependencies be updated as well?

https://github.com/styleguidist/react-docgen-typescript/pull/360/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R26

@pvasek
Copy link
Collaborator

pvasek commented Jun 7, 2021

@michelengelen Thanks for your PR! I think in the past we used peerDependencies for typescript. Which was somehow later promoted to devDepencencies without notice. Which is fine I guess. Would you be so kind and remove it from peerDependencies?

Question for everyone:
I am not sure how to approach the next release version. Maybe the update to typescript 4.3 could be seen as a breaking change for this kind of library. What are your opinions about that? Any advice?

@michelengelen
Copy link
Contributor Author

@pvasek First: I did bump the typescript version in peerDependencies to >= 4.3.x. This does not affect the devDependencies, but will at least show a warning when installing in a project that does not meet the peerDependency.

As for your question: I do think as well that this might be a candidate for a breaking change, since the SymbolDisplayPart type is only available in TS versions 4.3.x and up. All version below that will probably not work anymore with the current state of the package.

That leaves us with 2 options:

  1. Either make the new version a major release to show that breaking changes have occured, or at least (when a major release is out of question) list it in the CHANGELOG as potentially breaking.
  2. I could add guards (or a handling function) around every occurence of SymbolDisplayPart within the parser.

WDYT?

@pvasek
Copy link
Collaborator

pvasek commented Jun 9, 2021

@michelengelen maybe it's "easier" to go with the first option. I think it's at least safe. Maybe not most convenient for everyone but people using a new typescript should be able to update this easily.

@pvasek pvasek merged commit 8917fb2 into styleguidist:master Jun 9, 2021
@Methuselah96
Copy link

Methuselah96 commented Jun 9, 2021

@pvasek Is it worth releasing one more version of the package without the fix and tighten the peer dependency range to make it clear that it's not compatible with TypeScript 4.3? Right now it's >= 3.x.

phildenhoff added a commit to phildenhoff/cli-connect-react-plugin that referenced this pull request Nov 18, 2022
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.

TypeError: (tag.text || "").trim is not a function
6 participants