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

JS SDK support #1862

Merged
merged 15 commits into from
Aug 8, 2022
Merged

JS SDK support #1862

merged 15 commits into from
Aug 8, 2022

Conversation

maxhr
Copy link
Contributor

@maxhr maxhr commented Aug 2, 2022

  • Major refactor
  • JS SDK
  • CLI user prompts

@maxhr maxhr marked this pull request as ready for review August 2, 2022 06:36
@maxhr maxhr requested a review from volovyks as a code owner August 2, 2022 06:36
- JS SDK
- CLI user prompts
@maxhr maxhr marked this pull request as draft August 2, 2022 07:39
@volovyks
Copy link
Contributor

volovyks commented Aug 2, 2022

Since it will be the main way to create JS contract, let's add instructions here: near/near-sdk-js#145

Copy link
Contributor

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

The fact that CI does not work for some people in our org is annoying. Why is this happening?

package.json Outdated Show resolved Hide resolved
.github/workflows/test-matrix.yml Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
src/app.ts Outdated Show resolved Hide resolved
src/checks.ts Outdated Show resolved Hide resolved
src/make.ts Outdated Show resolved Hide resolved
@maxhr
Copy link
Contributor Author

maxhr commented Aug 2, 2022

The fact that CI does not work for some people in our org is annoying. Why is this happening?

@volovyk-s it still breaks on M1 because of workspaces-rs. Is that what you mean?

@gagdiez
Copy link
Collaborator

gagdiez commented Aug 2, 2022

@maxhr I think @volovyk-s means that actions are not being triggered, and that's because the PR comes from another repo. Maybe we should migrate the branch here.

@maxhr
Copy link
Contributor Author

maxhr commented Aug 2, 2022

Yes, forks don't trigger actions on the original. I don't have permissions to push here.

@volovyks
Copy link
Contributor

volovyks commented Aug 3, 2022

The fact that CI does not work for some people in our org is annoying. Why is this happening?

@volovyk-s it still breaks on M1 because of workspaces-rs. Is that what you mean?

I meant what @gagdiez said.
Regarding M1 - it should work, both Workspaces RS and JS support M1 on the latest version. But those versions are breaking Mac CI (known issue: near/nearcore#7235).

test/make.test.ts Outdated Show resolved Hide resolved
@maxhr maxhr marked this pull request as ready for review August 3, 2022 18:15
Copy link

@idea404 idea404 left a comment

Choose a reason for hiding this comment

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

Looks good. A couple non-blocking comments:

  1. Testing - If I read correctly, there is no test in place right now to test recommending WSL for Windows environments. Maybe good to have something like this in place. It would also be nice for the e2e tests to have a summary view once they have run, they produce a lot of output on the terminal atm which is hard to follow.
  2. Dependency versioning management - Every template comes with dependencies and each their own versions. I believe it would make the repo easier to maintain if these versions would be specified in one file and these values would be extracted when dApp is created by user.
  3. Config management / maintainability - Similar to the point above, I notice the supported languages/frameworks are specified multiple times throughout multiple files in the repo, this can get painful to manage as we add support for more languages and frameworks in the future. I would suggest using a config file and specifying these in the config file and have the rest of the repo read this file.

Also, npm run test fails for the js tests for me in my ubuntu machine.

package.json Show resolved Hide resolved
src/tracking.ts Show resolved Hide resolved
const optionsAreValid = hasAllOptions
&& ['react', 'vanilla', 'none'].includes(frontend)
&& ['js', 'rust', 'assemblyscript'].includes(contract)
&& ['js', 'rust'].includes(tests);
Copy link

Choose a reason for hiding this comment

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

I've seen definitions like these here and in types.ts (and probably still other parts), would it be perhaps better to define these in a separate config.json file or similar for the entire project once and fetch those definitions from that file instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

templates/contracts/js/package.json Outdated Show resolved Hide resolved
@gagdiez
Copy link
Collaborator

gagdiez commented Aug 8, 2022

@volovyk-s this is ready to be merged

@volovyks volovyks merged commit a00488e into near:master Aug 8, 2022
@volovyks
Copy link
Contributor

volovyks commented Aug 8, 2022

@gagdiez ok
It is better to create releases from the master. It automatically adds tags and a release here on GitHub. Now we need to do that manually.
And do not forget to use conventional commits in this repo :)

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