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: Don't use global Buffer object #733

Merged
merged 3 commits into from
Jan 17, 2023
Merged

Conversation

jasonpaulos
Copy link
Contributor

@jasonpaulos jasonpaulos commented Jan 13, 2023

Summary

This PR makes the SDK no longer rely on a global Buffer object. See #708 for more context and for why this caused undesirable behavior.

I'm proud to report that this PR allows the SDK to be installed in a create-react-app project without ejecting or any additional configuration changes.*

Specifics

My comment in #730 (comment) expands on how our previous behavior causes problems for create-react-app.

Now, instead of using the global Buffer variable in our code and relying on a webpack plugin to set it up properly, we explicitly import Buffer from the buffer package. On node, this gives us the same thing as the global Buffer variable, and on the browser, this gives us our in-browser buffer package.

Note: the fact that the npm browser package is named buffer is convenient but not necessary. We could move to a differently named package if we wanted to (and we might, since buffer has not been updated in some time). We'd just need to add the field "buffer": "other_package_name" to the browser object in our package.json and webpack will be able to substitute the new package when we import buffer.

In order to enforce this change in our repo, I made some changes to our eslint rules. The new rule no-restricted-globals causes eslint to fail if it detects any reference to the global Buffer variable, which should catch any problems going forward.


*There's one situation which unfortunately we can't solve in this SDK. Webpack v4 projects forcibly import an old version of the npm buffer package. Despite npm installing the higher version of buffer that we've specified in our package.json, webpack gives us the old one. I've given up trying to get around this for now, and I've changed the offending code causing #730 to use DataViews instead of Buffer. This solves the immediate problem, but it's possible other issues may arise from using an older version of buffer with our library.

I've opened #734 as a possible longer term solution to this problem.

Comment on lines +4 to +11
'shared-node-browser': true,
},
extends: ['airbnb-base', 'prettier', 'plugin:import/typescript'],
extends: [
'airbnb-base',
'prettier',
'plugin:import/typescript',
'eslint:recommended',
],
Copy link
Contributor Author

@jasonpaulos jasonpaulos Jan 13, 2023

Choose a reason for hiding this comment

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

These changes aren't explicitly necessary, but I thought I would include them since I'm making changes to our eslint settings.

  • shared-node-browser causes eslint to only recognize global variables present in both node and browser contexts, instead of either.
  • I removed mocha from the global list of environments in favor of adding /* eslint-env mocha */ at the top of each mocha test file. This way we won't accidentally reference mocha global variables from our production files.
  • eslint:recommended contains a set of recommended rules. It only caused one new issue in our codebase (which I fixed by configuring no-constant-condition below), so I figured it was a good idea to include.

@jasonpaulos jasonpaulos changed the title Don't use global Buffer object Packaging: Don't use global Buffer object Jan 13, 2023
@jasonpaulos jasonpaulos mentioned this pull request Jan 14, 2023
@barnjamin
Copy link
Contributor

FWIW I just tested against a fresh create-react-app and used npm link against this pr branch and needed no config changes to include and use the algosdk (yay!)

Copy link
Contributor

@barnjamin barnjamin left a comment

Choose a reason for hiding this comment

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

shipit!

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.

The buffer package recommends importing buffer with a trailing slash - do we also want to do that here?

@jasonpaulos
Copy link
Contributor Author

@algochoi good question. In this case we want to import just buffer (no trailing slash) so that when running on Node, Node's native Buffer implementation will be used.

In fact I wanted to specify this more explicitly by importing node:buffer, but webpack didn't like that usage.

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.

👍

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.

3 participants