Skip to content

Conversation

@maxwellpothier
Copy link
Contributor

This is branched off the changes created by @akadoshin here: #90

package.json Outdated
},
"devDependencies": {
"@babel/preset-env": "^7.25.4",
"@rollup/plugin-commonjs": "^26.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge deal, but there are newer versions of this package available that we should use. The version listed here has a bunch of warnings for the glob versions used. If you update to v28 those warnings go away

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the other example files also be renamed to .mjs? Or is just one needed to make sure rollup is working correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary, as the other examples work fine with CommonJS. However, if you'd like to add new examples using .mjs, I’m not opposed to it.

Makefile Outdated
build:
npm run build

publish: test version upload unversion
Copy link
Contributor

Choose a reason for hiding this comment

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

the build command should be added as part of the publish command. I think

publish: test version build upload unversion should work?

In addition, we should define a .npmignore file with the following lines in order to not publish unecessary files npm (not for privacy reasons, but just for keeping things tidy)

src/
tests/
node_modules/
examples/

.env
.vscode/
.idea/
.DS_Store
.eslintignore
.eslintrc.json
.git/
.github/
.gitignore
.prettierignore
.prettierrc

*.log

You can test out what will be published to npm with npm publish --dry-run

Copy link
Contributor

Choose a reason for hiding this comment

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

@camiblanch @maxwellpothier I agree with reducing the package size. When I first looked at the project, I wasn't sure why all the code was being exported in the package, so I left it as it was. However, in this case, it would be better to specify which files to include rather than excluding them. In the package.json, you can do something like this:

{
  "files": [
    "dist",
    "README.md",
    "LICENSE"
  ]
}

This way, you’re only including the necessary build folder (dist), which is meant for publishing the package to npm, and other relevant files like the README and license.

@maxwellpothier
Copy link
Contributor Author

@camiblanch @akadoshin

I've made those changes to update our dependencies and publishing process.

I originally upgraded chai to v5, but they released breaking changes that no longer support commonjs. I've made a task in our backlog to get this handled, because this change is out of the scope of this PR. The chai version is still the same here.

Copy link
Contributor

@camiblanch camiblanch left a comment

Choose a reason for hiding this comment

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

Looks great!

@maxwellpothier maxwellpothier merged commit 7799a36 into master Oct 24, 2024
3 checks passed
@maxwellpothier maxwellpothier deleted the max/esmodules branch October 24, 2024 16:42
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.

Runtime error with Axios using SDK in React

4 participants