Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

fix: do not minify compiled sources for Node.js #530

Merged
merged 3 commits into from
Aug 4, 2022
Merged

fix: do not minify compiled sources for Node.js #530

merged 3 commits into from
Aug 4, 2022

Conversation

robertrossmann
Copy link
Contributor

Minification makes debugging this project in its installed form practically impossible because

  1. you cannot reasonably step through the code
  2. variable names are unreadable

I understand the motivation to minify the codebase when targetting web browsers to reduce the overal size of the SDK but for Node.js file size is only a problem when it goes to the extremes.

I would like to also ask you, should you accept this PR, to also publish a non-minified version of the SDK to npm. 🙏

Minification makes debugging this project in its installed form practically impossible because

1. you cannot reasonably step through the code
2. variable names are unreadable

I understand the motivation to minify the codebase when targetting web browsers to reduce the overal size of the SDK but for Node.js file size is only a problem when it goes to the extremes.

I would like to also ask you, should you accept this PR, to also publish a non-minified version of the SDK to npm. 🙏
@joaquim-verges
Copy link
Member

Thanks for this @robertrossmann !

This change seems very reasonable to me. We need to run a couple of tests to check what happens on some browser frameworks that annoyingly load the node package instead of the browser one 😐

Will let you know once it's done, we'll try to get to it this week. cc @jnsdls

@robertrossmann
Copy link
Contributor Author

Thanks @joaquim-verges ! ❤️ FYI merging and releasing this to npm would be a great help on my current project as it would allow me to step through the SDK's code when things don't work as I would expect. 😄 Also, this would allow me to apply hotfixes on my local installation of the SDK with patch-package and submit a proper pull request at a more convenient time than two days before product launch. 😂

@joaquim-verges
Copy link
Member

Merging this as well. Haven't had time to test much but it's easy to revert if reports come through.

@joaquim-verges joaquim-verges merged commit 424c7ad into thirdweb-dev:main Aug 4, 2022
@robertrossmann robertrossmann deleted the fix/no-minified-node branch August 4, 2022 20:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants