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

Updates from near-api-js #105

Merged
merged 12 commits into from
Nov 30, 2020
Merged

Updates from near-api-js #105

merged 12 commits into from
Nov 30, 2020

Conversation

volovyks
Copy link
Contributor

No description provided.

@volovyks volovyks requested review from ailisp and frol as code owners November 23, 2020 20:24
@volovyks volovyks marked this pull request as draft November 23, 2020 20:25
Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Is there specific reason to do these renames? I'm curious about why rename here instead of export this and import and change usage on near-api-js side, since camelCase is more common in js

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

I have the same concern about the snake case in JS code (while I prefer it, I still follow the style that is common in the target language, and camelCase is what used in JS).

@vgrichina Also, I wonder if we can remove borsh-js build from the repo? I usually find it to be a source of merge conflicts, bloating the sources, and inconsistencies if contributors forget to build or include the generated files in their PR.

@@ -1,4 +1,4 @@
const borsh = require('../lib/index.js');
const borsh = require('../../borsh-js/index.js');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a hack. Can we use ts? Can we write the tests in TS?

@frol
Copy link
Collaborator

frol commented Nov 24, 2020

@volovyk-s I want to point out that this repo also contains the Rust implementation of Borsh, and thus lib folder in the root is a bit misleading.

@ailisp @evgenykuzyakov @nearmax Are there any objections to renaming this repo to borsh-rs, extracting the JS/TS implementation into borsh-js repository, and the landing page to borsh repository?

@volovyks
Copy link
Contributor Author

Is there specific reason to do these renames? I'm curious about why rename here instead of export this and import and change usage on near-api-js side, since camelCase is more common in js

@ailisp @frol Yep, we are using camel case everywhere, so I have changed it back. I'm not sure why these changes were added in near-api-js

@volovyks
Copy link
Contributor Author

@volovyk-s I want to point out that this repo also contains the Rust implementation of Borsh, and thus lib folder in the root is a bit misleading.

@ailisp @evgenykuzyakov @nearmax Are there any objections to renaming this repo to borsh-rs, extracting the JS/TS implementation into borsh-js repository, and the landing page to borsh repository?

It was renamed because borsh-js is also confusing. It's not a separate JS implementation, it's just a build artifact of TS subproject. BTW, @frol , JS artifacts can not be deleted, because some developers can use this repo instead of NPM package (like here).

It would be nice to move TS/JS version to a separate repo.
Pros:

  • no confusions with lib or borsh-js folders (thnx @frol )
  • it's confusing for the Rust developers to see ~10 unrelated files in the root directory
  • separated CI checks
  • separate README.md in the root folder (easier onboarding of new users)
    Cons:
  • partially destroyed Git history

@frol
Copy link
Collaborator

frol commented Nov 24, 2020

Cons: partially destroyed Git history

Git history can be preserved if we just clone this repo and clean up unrelated parts as a regular commit. Yes, we will have borsh-rs in the history in two places, but overall it should be fine.

@ailisp
Copy link
Member

ailisp commented Nov 24, 2020

I also vote for separate borsh-rs

@volovyks volovyks marked this pull request as ready for review November 25, 2020 07:10
@vgrichina
Copy link
Contributor

Cons: partially destroyed Git history

Git history can be preserved if we just clone this repo and clean up unrelated parts as a regular commit. Yes, we will have borsh-rs in the history in two places, but overall it should be fine.

can be done even better with git-filter-branch https://manishearth.github.io/blog/2017/03/05/understanding-git-filter-branch/ (will result in smaller repo, but history essentially will be rewritten)

.eslintrc.yml Outdated
- error
- always
no-console: off
globals:
Copy link
Contributor

Choose a reason for hiding this comment

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

@volovyk-s let's keep only actually necessary globals for borsh

@frol
Copy link
Collaborator

frol commented Nov 25, 2020

@volovyk-s May I ask you to extract the relevant history into https://github.com/near/borsh-js and https://github.com/near/borsh-rs? (Just push master branches there and I will switch the default from main to master branch on Github)

@volovyks
Copy link
Contributor Author

@volovyk-s May I ask you to extract the relevant history into https://github.com/near/borsh-js and https://github.com/near/borsh-rs? (Just push master branches there and I will switch the default from main to master branch on Github)

Ok, I will do that after this merge.

@volovyks volovyks merged commit 4c2f1a0 into master Nov 30, 2020
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.

4 participants