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

Docs: Added jsDoc #144

Merged
merged 8 commits into from
Feb 1, 2024
Merged

Docs: Added jsDoc #144

merged 8 commits into from
Feb 1, 2024

Conversation

jankapunkt
Copy link
Member

Summary

Adds jsDoc to build and code

Linked issue(s)

partially #141 in order to improve documentation for better logging

Involved parts of the project

  • build
  • all code
  • Readme
  • docs

Added tests?

no extra tests

Targeted Meteor release version

Reproduction

clone repo, checkout docs/jsdoc and run npm run build:docs

@jankapunkt
Copy link
Member Author

published a release 2.8.0-rc.1 for testing this PR

@bratelefant
Copy link
Collaborator

I do some testing with 2.8.0-rc.1.

@bratelefant
Copy link
Collaborator

Got a little trouble building with expo, because I also import @meteorrn/local and the depencency with 2.8.0-rc.1 is not accepted. Can I override (--force?) this in eas-cli?

@jankapunkt
Copy link
Member Author

Is there a specific error?

@bratelefant
Copy link
Collaborator

If I do npm install @meteorrn/local@2.8.0-rc.1 I end up with No matching version found for @meteorrn/local@2.8.0-rc.1.

@jankapunkt
Copy link
Member Author

jankapunkt commented Jan 24, 2024

You have to npm install @meteorrn/core@2.8.0-rc.1 , it's published to NPM so you don't need a local version.

Edit: since it's tagged as next on npm it's only installed if you explicitly set the version and the defualt on NPM still points to 2.7.1

@bratelefant
Copy link
Collaborator

bratelefant commented Jan 24, 2024

I get @meteorrn/core@2.8.0-rc.1 installed, but while building with eas-cli I get

Could not resolve dependency:
[INSTALL_DEPENDENCIES] npm ERR! peer @meteorrn/core@">=2.0.7" from @meteorrn/local@1.0.3
[INSTALL_DEPENDENCIES] npm ERR! node_modules/@meteorrn/local
[INSTALL_DEPENDENCIES] npm ERR!   @meteorrn/local@"^1.0.3" from the root project

because I also have @meteorrn/local@1.0.3 in my package.json. Is @meteorrn/core@">=2.0.7 not fulfilled?

@jankapunkt
Copy link
Member Author

ah I see, this is a peer dependency conflict, let me check, if I can resolve this quickly

@bratelefant
Copy link
Collaborator

Can we simply add a >= 2.0.7 || 2.8.0-rc.1 or something as a peer dep for meteorrn/local?

@bratelefant
Copy link
Collaborator

Could build it with the added peerDep

Copy link
Collaborator

@bratelefant bratelefant left a comment

Choose a reason for hiding this comment

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

Docs are great, maybe at some point we could add a git workflow to auto-generate docs and deploy on github pages

@bratelefant
Copy link
Collaborator

I'd like to report that removing NetInfo in my staging environment seems to solve a problem that occurred randomly and is still hard to reproduce.

Namely, on iOS, when bringing the app back up from (suspended) background, the user did not log back in, although the socket was connected.

Now I manage connection / disconnecting via rn AppState, look good so far.

@jankapunkt
Copy link
Member Author

Great! We should add that to the docs, like a troubleshooting section. Something like "if you have trouble with reconnecting,.try Appstate instead of NetInfo. Here is a minimal example:...". Do you have the time and will to add this section to this PR?

@jankapunkt
Copy link
Member Author

@bratelefant we will add Troubleshooting / FAQ in another PR, would you mind adding a review in the meantime?

Copy link
Collaborator

@bratelefant bratelefant left a comment

Choose a reason for hiding this comment

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

Sorry, forgot to approve. Changes reviewed and looking good.

@jankapunkt jankapunkt merged commit c77f0b6 into master Feb 1, 2024
9 checks passed
@bratelefant
Copy link
Collaborator

We can merge this, will add my AppState example in another PR

@jankapunkt
Copy link
Member Author

Thanks 👍

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.

Expo app crashes right after launch due to Meteor.connect()
2 participants