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

Distribution generates CommonJS and ES6, removal of bs58 dependency (not used in code) #48

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ochaloup
Copy link
Contributor

@ochaloup ochaloup commented May 24, 2023

Changing the distribution to be run with both commonjs and es6 module types.
Webpack minified version is left intact.

@ochaloup ochaloup requested review from AlexStefan and MjCage May 24, 2023 07:45
@ochaloup
Copy link
Contributor Author

Just wondered if makes sense to have build different module versions. Similar to how it's referred at e.g., here https://stackoverflow.com/a/58724355/187035

@ochaloup
Copy link
Contributor Author

I would like to ask for re-review @AlexStefan @MjCage

What I did. I checked the jupiter, mango and anchor how they build their packages. From that it can be seen there is normal to build both types of packages - esm and commonjs (https://github.com/blockworks-foundation/mango-v4/blob/dev/package.json#L19). When I tried without commonjs on my testing project https://github.com/ochaloup/marinade-ts-sdk-example - which is a nodejs project - I've got issues to get it running (while I'm not still an expert to say that I could not be missing some conf that it could work without having that type of module, anyway).

The build process now generates the both types of modules and both types of modules in minified version with webpack.

I think this part of the update is fine to be reviewed and in case merged.

(Where I still fight at is the tree-shaking process for a dependent library. I'm not able to make tree shaking to work when using marinade-ts-sdk in my test project.)

@ochaloup ochaloup marked this pull request as draft May 29, 2023 07:08
@ochaloup
Copy link
Contributor Author

I'm converting this to Draft. I need a little bit more discussion on this topic before finalizing the PR.

@ochaloup ochaloup marked this pull request as ready for review May 30, 2023 07:30
@ochaloup
Copy link
Contributor Author

I changed the PR and it's ready for review.

We discussed that the work consists of two parts

  1. module distribution (covered by this change, ready for review). Inspiration came from mango-v4 repository and other projects.
  2. refactoring of the code to be more tree shakable and consists of functions rather than all covered in classes. this part need more investigation and discussions.

@ochaloup ochaloup changed the title Module type from CommonJS to ES6, removal of bs58 dependency (not used in code) Distribution generates CommonJS and ES6, removal of bs58 dependency (not used in code) May 30, 2023
@ochaloup ochaloup marked this pull request as draft June 27, 2023 06:56
@ochaloup
Copy link
Contributor Author

I changed this PR to draft as the work was put on hold and some more points are needed to be done on top of this drafted PR.

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.

1 participant