-
Notifications
You must be signed in to change notification settings - Fork 9
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
Allow specifying SWC version #54
Conversation
7d943cf
to
34f1783
Compare
r? @maelanleborgne would you mind checking this PR? thank you! |
Very nice PR @pan93412 ! I'm just a little worried that this is too big of a change to be forced over users : it will force the download of a new binary, that could be a problem on established production setups (will the permissions be ok ? There's no cleanup of the previous binary. And are there any breaking changes between 1.3.92 and 1.7.26 ? ...) Do you think PR could be done at first without the binary file renaming ? So that users who don't need to update the binary won't be affected by the update, and maybe write in the docs that, in order to upgrade the binary version, you must delete it first then run the command (that's just a suggestion). |
Good idea. Let me revert the filename commits and the SWC version changes. Thanks for your suggestion! Old tag: 34f1783 |
34f1783
to
9a29efd
Compare
9a29efd
to
f4d5a84
Compare
I have reverted the version and renaming part, and documented in order to upgrade the binary version, you must delete it first then run the command in the documentation. Would you mind checking this again? Thank you! |
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.
Just a few things but otherwise it looks good to me. I'll make some tests on my side to validate this, but I think it can be release today
Co-authored-by: maelanleborgne <[email protected]>
Everything seem to run smoothly, just one last thing : I think we should throw an exception right away if the response isn't successful. I'd suggest adding somthing like this in TypeScriptBinaryFactory:129 just after the request is made : if (200 !== $statusCode = $response->getStatusCode()) {
$exceptionMessage = \sprintf('Could not download SWC binary from "%s" (request responded with %d).', $url, $statusCode);
if (404 === $statusCode) {
$exceptionMessage .= PHP_EOL.\sprintf('Check that the version "%s" defined in "sensiolabs_typescript.swc_version" exists.', $this->swcVersion);
}
throw new \Exception($exceptionMessage);
} |
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.
LGTM, thank you very much for this feature @pan93412 🙏
Fix #53.
swc_version
in the configuration YAML file.By default, it uses the (current) latest SWC version v1.7.26:
Users can specify it in
config
:and AssetMapperTypeScriptBundle will download and use the user-picked version.
Of course, the downloaded binary is still cached.