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

feat: Add LiveQuery module to SDK #1712

Merged
merged 17 commits into from
Jun 8, 2023

Conversation

vazarkevych
Copy link
Contributor

@vazarkevych vazarkevych commented Feb 24, 2023

New Pull Request Checklist

Issue

Closes: #1714

TODOs before merging

  • Added module ParseLiveQuery to SDK project
  • Added SPM support for the module ParseLiveQuery
  • Added build module ParseLiveQuery from CI/CD (this change needs to check because the test finished with an error. Locally everything works but on the repo we have an error)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Feb 24, 2023

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@mtrezza mtrezza changed the title WIP: feat: Add module LiveQuery to Parse SDK project feat: Add module LiveQuery to Parse SDK project Feb 24, 2023
@mtrezza mtrezza marked this pull request as draft February 25, 2023 17:44
@mtrezza
Copy link
Member

mtrezza commented Mar 10, 2023

@vazarkevych Could you give a description of the issues that are stopping this PR currently?

And could you please do a rebase to resolve the conflicts? Maybe the latest workflow changes address some of the issues. If you have any questions regarding the new workflow, maybe @dplewis could give a hand...

@dplewis
Copy link
Member

dplewis commented Mar 10, 2023

@vazarkevych I can give a hand, can you give me access to your fork?

@vazarkevych
Copy link
Contributor Author

@dplewis I gave access you to my fork

@vazarkevych
Copy link
Contributor Author

Hi, @dplewis, Just a kind follow-up regarding the status of the ParseLiveQuery. Do you have any updates on your side?

@dplewis
Copy link
Member

dplewis commented Mar 30, 2023

@vazarkevych I fixed the merge conflict waiting for CI to pass

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (cbab34c) 78.17% compared to head (413aec7) 78.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1712      +/-   ##
==========================================
- Coverage   78.17%   78.16%   -0.01%     
==========================================
  Files         307      307              
  Lines       36803    36803              
==========================================
- Hits        28770    28768       -2     
- Misses       8033     8035       +2     

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dplewis
Copy link
Member

dplewis commented Mar 30, 2023

@vazarkevych I got the live query project to build successfully, looks like the release build is broken

@dplewis
Copy link
Member

dplewis commented Mar 31, 2023

@vazarkevych I got the release working but ParseLiveQuery-OSX is broken. I don't know why BoltsSwift is compiling to macOS 10.10 in the CI and locally for me.

❌  /Users/runner/work/Parse-SDK-iOS-OSX/Parse-SDK-iOS-OSX/ParseLiveQuery/ParseLiveQuery/Internal/ClientPrivate.swift:13:8: compiling for macOS 10.10, but module 'BoltsSwift' has a minimum deployment target of macOS 10.15: /Users/runner/work/Parse-SDK-iOS-OSX/Parse-SDK-iOS-OSX/build/macOS/Release-osx-macosx/BoltsSwift.framework/Modules/BoltsSwift.swiftmodule/x86_64-apple-macos.swiftmodule

@dplewis
Copy link
Member

dplewis commented Mar 31, 2023

@vazarkevych @mtrezza The project builds , the examples compile and run, and the CI is working. I'm not familiar with how SPM works, can you check it?

@dplewis dplewis requested a review from a team March 31, 2023 21:29
@mtrezza
Copy link
Member

mtrezza commented Apr 2, 2023

@vazarkevych Could you look into the remaining issues so we can get this merged?

@vazarkevych
Copy link
Contributor Author

Hi @dplewis, thank you for your contribution. I just checked SPM, and it stopped working

@dplewis
Copy link
Member

dplewis commented Apr 5, 2023

@vazarkevych I don't think I changed anything SPM related.

@dplewis
Copy link
Member

dplewis commented Apr 12, 2023

@vazarkevych @mtrezza I got this to work via SPM. I added ParseObjC and ParseLiveQuery to my project from your github url and addParseLiveQuery branch. Then used

#import "Parse.h"
@import ParseLiveQuery;

I just checked SPM, and it stopped working

What stopped working?

dplewis
dplewis previously approved these changes Apr 13, 2023
Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Documentation, README, code coverage, starter projects, demo, and tests can be done in separate PRs. Also there are still PR's left on the LiveQuery repo that need to be addressed

@mtrezza
Copy link
Member

mtrezza commented Apr 13, 2023

Amazing! Maybe we could at least copy/paste the LiveQuery README to this README under a new chapter? Or is that content now outdated because of the migration?

@dplewis
Copy link
Member

dplewis commented Apr 13, 2023

That documentation is outdated and very hard to get working for new users, I can't get it working. Once the project is migrated the LiveQuery code needs some TLC

@mtrezza
Copy link
Member

mtrezza commented Apr 13, 2023

Got it, then let's just wait for a review by @vazarkevych to confirm all is working as expected and then merge.

@mtrezza
Copy link
Member

mtrezza commented Apr 13, 2023

Anyone could try this out and confirm the LiveQuery module is working?

@parse-community/ios-sdk @extnous @HackShitUp

@mtrezza mtrezza marked this pull request as ready for review April 14, 2023 09:00
Package.swift Outdated
@@ -10,19 +10,22 @@ let package = Package(
.tvOS(.v12),
.watchOS(.v2)],
products: [
.library(name: "ParseObjC", targets: ["ParseCore"]),
.library(name: "ParseObjC", targets: ["Parse"]),
Copy link
Member

Choose a reason for hiding this comment

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

Why this change and is this a breaking change?

It seems a bit odd to name the core module Parse, as the term "Parse" is arguably referring to the whole SDK. To me is makes more sense to name the core module ParseCore to indicate that it's the basis that is always needed, while others are optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, yes it is an important change because in SDK target has the name "Parse". When we set "ParseCore" it causes an error downloading.

Copy link
Member

@mtrezza mtrezza Apr 24, 2023

Choose a reason for hiding this comment

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

Not sure I understand. Why can't we keep the name ParseCore? I mean this is only an issue for the LiveQuery module, right? Otherwise we make a breaking change because developers changed their imports from Parse to ParseCore with the introduction of SPM, and now we are changing it back to Parse.

Copy link
Member

@dplewis dplewis May 9, 2023

Choose a reason for hiding this comment

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

@mtrezza This is my first time using SPM and I didn't know you had to do import ParseCore;. I can try to revert it back and fix the downloading error. @vazarkevych Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@vazarkevych Could you take a look at this so we can close this PR? It seems to be the last remaining issue here.

Copy link
Member

Choose a reason for hiding this comment

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

@mtrezza Can you commit 1 example of tedious work? I'll finish the rest. I don't quite understand what needs to be changed

Copy link
Member

@mtrezza mtrezza May 27, 2023

Choose a reason for hiding this comment

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

See #1712 (comment). The module name ParseCore shouldn't change, just because we're adding the LiveQuery module in this PR. Otherwise this PR becomes a breaking change, but it's just a feature addition.

So I'd simply remove the renaming and then go into the LiveQuery module and make sure it uses the ParseCore reference. It seems the issue is that the LiveQuery module is looking for the Parse module by using its old Parse reference instead of the ParseCore reference in this repository. To me this looks like a simple find/replace task and some manual correction.

This comment was marked as off-topic.

This comment was marked as off-topic.

Copy link
Member

Choose a reason for hiding this comment

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

@dplewis Do you want to take a look at this, we could add an additional bounty.

Cartfile.resolved Show resolved Hide resolved
@dplewis
Copy link
Member

dplewis commented Jun 7, 2023

@mtrezza @vazarkevych @VolodyaNazarkevych This is ready for review.

TODO: #1712 (review)

@mtrezza
Copy link
Member

mtrezza commented Jun 7, 2023

Amazing! @VolodyaNazarkevych Could you please take a look at this and test this module out in a project?

@mtrezza mtrezza changed the title feat: Add module LiveQuery to Parse SDK project feat: Add LiveQuery module to Parse SDK Jun 8, 2023
@mtrezza mtrezza changed the title feat: Add LiveQuery module to Parse SDK feat: Add LiveQuery module to SDK Jun 8, 2023
@mtrezza mtrezza merged commit 154da34 into parse-community:master Jun 8, 2023
parseplatformorg pushed a commit that referenced this pull request Jun 8, 2023
# [2.3.0](2.2.0...2.3.0) (2023-06-08)

### Features

* Add LiveQuery module to SDK; this deprecates the separate [Parse LiveQuery SDK](https://github.com/parse-community/ParseLiveQuery-iOS-OSX) ([#1712](#1712)) ([154da34](154da34))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 2.3.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add LiveQuery as module
6 participants