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

remove transpilation of companion #4669

Closed
wants to merge 1 commit into from
Closed

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Sep 6, 2023

this was discussed before but we never got to it:

why? transpilation adds junk to the source code making it hard to debug issues and stack traces in production because line numbers become messed up also we don't need to transpile because we are targeting relatively recent node versions so stuff like generator-runtime isn't needed anymore.

example: https://www.unpkg.com/@uppy/companion@4.5.1/lib/server/provider/credentials.js

alternatively close this PR and instead merge #4670

why? transpilation adds junk to the source code
making it hard to debug issues and stack traces in production because line numbers become messed up
also we don't need to transpile because we are targeting relatively recent node versions
so stuff like generator-runtime isn't needed anymore
example: https://www.unpkg.com/@uppy/companion@4.5.1/lib/server/provider/credentials.js
@mifi mifi mentioned this pull request Sep 6, 2023
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Potentially breaking change because importing @uppy/companion/lib/whatever will no longer work (because the lib/ folder is no more), but probably something we want to do at this point.

If we make a new major for Companion, we should also remove support for Node.js 14.x and 16.x which are EOL as of today.

@arturi
Copy link
Contributor

arturi commented Sep 12, 2023

How about Typescript though? If we switch Companion to TS, we'll have lib folder and a compilation step again.

@aduh95
Copy link
Contributor

aduh95 commented Sep 12, 2023

That's a good point, we probably want to do that indeed.

@kvz
Copy link
Member

kvz commented Sep 12, 2023

Agreed, ts-fly is fun for Transloadit but not for the world

@aduh95
Copy link
Contributor

aduh95 commented Nov 30, 2023

Closing as #4670 has merged

@aduh95 aduh95 closed this Nov 30, 2023
@aduh95 aduh95 deleted the companion-remove-transpilation branch November 30, 2023 13:51
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.

4 participants