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

Packaging: Improve source map and browser usage for external bundlers #707

Merged
merged 4 commits into from
Dec 9, 2022

Conversation

jasonpaulos
Copy link
Contributor

@jasonpaulos jasonpaulos commented Dec 7, 2022

While working on https://github.com/jasonpaulos/decipher-22-pyteal-talk, I became aware of a few deficiencies of using this SDK in React. This PR attempts to improve most, but not all, of the issues I encountered. They are:

  • Missing sources: while we generate and ship source maps with our package, we don't include the actual sources, which make the source maps essentially useless since they have nothing to map to. To fix this, I've included the src directory in our release bundle.
    • As an additional improvement that wasn't strictly necessary, I moved the index.ts file into the src directory. This helps simplify the directory structure for our sources and compiled code. As far as I'm aware there's no downside to doing this.
  • Failing to import the crypto module: as I explained in this comment, our dependency tweetnacl-js attempts to import the crypto module in all environments to see if it's available, but that module is not necessary in the browser. Due to the strict nature of bundlers, this commonly caused an error. However, by modifying the browser field in our package.json, we can declare that that module should be ignored by browser bundlers.
  • Building on Node v18: our version of webpack has a bug that prevents it from building on newer versions of node, so I updated it.
    • While I was updating packages I also updated chromedriver, but this isn't strictly necessary.

The other issue I found but is not addressed in this PR is with regard to Buffer, and I've described that more in #708.

Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

Looks like good improvements:

As an additional improvement that wasn't strictly necessary, I moved the index.ts file into the src directory. This helps simplify the directory structure for our sources and compiled code.

Just checking since the PR makes changes to local paths: this won't affect how external users use the module correct?

@jasonpaulos
Copy link
Contributor Author

That's correct, the path changes will not affect what's exposed to users. The files referenced from our package.json are unchanged.

Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@jasonpaulos jasonpaulos merged commit 46f10af into develop Dec 9, 2022
@jasonpaulos jasonpaulos deleted the improve-browser-usage branch December 9, 2022 22:31
algochoi added a commit that referenced this pull request Jan 18, 2023
* ABI:  Refactor ABI encoding test to round-trip (#701)

* bump version and add to changelog

* update README.md for new version

* Packaging: Improve source map and browser usage for external bundlers (#707)

* bump version and add to changelog

* update README.md for new version

* v2: Make breaking changes from v1 to v2.0.0  (#717)

* bump version and add to changelog

* update README.md for new version

* remove enhancement section of recent changelog

* Enhancement: Add foreign array objects to ATC `addMethodCall` (#725)

* Add foreign array objects to ATC addmethodcall

* Copy array value so that inputs are not modified

Co-authored-by: Michael Diamant <[email protected]>
Co-authored-by: Lucky Baar <[email protected]>
Co-authored-by: Jason Paulos <[email protected]>
Co-authored-by: Jack Smith <[email protected]>
Co-authored-by: Barbara Poon <[email protected]>
algochoi added a commit that referenced this pull request Jan 18, 2023
* Add test for algod /v2/teal/disassemble

* Merge develop into PR (#736)

* ABI:  Refactor ABI encoding test to round-trip (#701)

* bump version and add to changelog

* update README.md for new version

* Packaging: Improve source map and browser usage for external bundlers (#707)

* bump version and add to changelog

* update README.md for new version

* v2: Make breaking changes from v1 to v2.0.0  (#717)

* bump version and add to changelog

* update README.md for new version

* remove enhancement section of recent changelog

* Enhancement: Add foreign array objects to ATC `addMethodCall` (#725)

* Add foreign array objects to ATC addmethodcall

* Copy array value so that inputs are not modified

Co-authored-by: Michael Diamant <[email protected]>
Co-authored-by: Lucky Baar <[email protected]>
Co-authored-by: Jason Paulos <[email protected]>
Co-authored-by: Jack Smith <[email protected]>
Co-authored-by: Barbara Poon <[email protected]>

* Explicitly import Buffer

* Revert test branch back to master

Co-authored-by: algochoi <[email protected]>
Co-authored-by: Lucky Baar <[email protected]>
Co-authored-by: Jason Paulos <[email protected]>
Co-authored-by: Jack Smith <[email protected]>
Co-authored-by: Barbara Poon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants