-
Notifications
You must be signed in to change notification settings - Fork 5
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
update SUSHI to v3.3.2 #232
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a question about updating the antlr version, but everything works as expected.
package.json
Outdated
@@ -67,7 +67,7 @@ | |||
"typescript": "^4.7.4" | |||
}, | |||
"dependencies": { | |||
"antlr4": "~4.8.0", | |||
"antlr4": "~4.13.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to include an antlr update here? I thought we wanted to continue to avoid updating to the version with performance issues since we did not yet need any other functionality from the updates. Did something change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try running the current version of GoFSH and this version against US Core and they both took about the same time, so maybe it doesn't make much of a difference. But I'm just curious if there was anything in particular that convinced you to update antlr4 to 4.13.0 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it's simpler to use the same version of antlr4
in both tools, but you're right that we don't really need any new features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I tested it out with US Core and don't see any changes and the speed is comparable. I also looked in node_modules and the top level antlr4
for GoFSH has the old version and no workaround is needed and the fsh-sushi/antlr4
has the workaround in place.
Completes task CIMPL-1151
This gets GoFSH up-to-date with SUSHI in preparation for updating both in FSH Online. Note that this does not update the
antlr4
dependency which was updated in SUSHI. GoFSH's usage ofantlr4
is sufficiently limited that it makes sense to wait until the performance improvement inantlr4
is available, at which point the dependency will be updated in both SUSHI and GoFSH.