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

Initial scheduler working prototype #1

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Attempt3035
Copy link

Love this project! It's exactly what my team needed! In our current project, we need scheduler, here's a basic chatGPT implementation (this has had literally 10 mins work so definitely not merge ready) which seems to be working. This PR is more an expression of interest and so other works can be coordinated, definitely will be spending some more time on this in the future and helping out making this project fully-fledged! Cheers!

Copy link
Member

@Rexios80 Rexios80 left a comment

Choose a reason for hiding this comment

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

I did an initial pass and this looks pretty good besides the doc comments. Please verify them all with the source material.

I will set up CI for analysis so we can ensure code style.

@@ -22,4 +23,8 @@ abstract final class FirebaseFunctions {
/// Access to the Firebase Functions Identity methods
static FirebaseFunctionsIdentity get identity =>
require('firebase-functions/v2/identity') as FirebaseFunctionsIdentity;

/// Access to the Firebase Functions Schedule methods
static FirebaseFunctionsSchedule get schedule =>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static FirebaseFunctionsSchedule get schedule =>
static FirebaseFunctionsScheduler get scheduler =>

@@ -22,4 +23,8 @@ abstract final class FirebaseFunctions {
/// Access to the Firebase Functions Identity methods
static FirebaseFunctionsIdentity get identity =>
require('firebase-functions/v2/identity') as FirebaseFunctionsIdentity;

/// Access to the Firebase Functions Schedule methods
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Access to the Firebase Functions Schedule methods
/// Access to the Firebase Functions Scheduler methods

/// Options that can be set on a schedule function.
extension type ScheduleOptions._(JSObject _) implements JSObject {
/// The schedule in Unix Crontab or AppEngine syntax.
external JSAny? get schedule;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
external JSAny? get schedule;
external String get schedule;

}

/// Event passed to a scheduled function handler.
extension type ScheduledEvent._(JSObject _) implements JSObject {
Copy link
Member

Choose a reason for hiding this comment

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

This should be above ScheduleOptions as it is in the source file

/// The Cloud Scheduler job name.
external String? get jobName;

/// The schedule time in RFC3339 UTC "Zulu" format.
Copy link
Member

Choose a reason for hiding this comment

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

This doc string does not match the source. All doc strings need checked for this since ChatGPT generated this.

});
}

/// The Firebase Functions Schedule namespace
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// The Firebase Functions Schedule namespace
/// The Firebase Functions Scheduler namespace

}

/// The Cloud Function type for Schedule triggers.
extension type ScheduleFunction._(JSObject _) implements JSObject {
Copy link
Member

Choose a reason for hiding this comment

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

This type declaration is unnecessary since Dart doesn't need to do anything with it


/// Constructor
external ScheduleOptions({
JSAny? schedule,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
JSAny? schedule,
String schedule,

/// Constructor
external ScheduledEvent({
String? jobName,
String? scheduleTime,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
String? scheduleTime,
String scheduleTime,

/// @param handler - A function to execute when triggered
/// @returns A function that you can export and deploy
external JSFunction onSchedule(
JSObject optionsOrSchedule, [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
JSObject optionsOrSchedule, [
JSAny args, [

JSObject does not include primitives

@Attempt3035
Copy link
Author

You legend thank you! I'll work through these over the next couple weeks as I get time, thank you! 🙏

@Rexios80
Copy link
Member

I added an analysis action, but for some reason I can't update this PR with main. Please merge with main so the action will run.

@Rexios80
Copy link
Member

@Attempt3035 are you planning on coming back to this?

@Attempt3035
Copy link
Author

Yeah planning to sorry 😅
If you are going to do it or want to close the PR for now that's all good, sorry it's taking me so long to get back to it!

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.

2 participants