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

chore(jobs): Mostly nitpicky changes to the Jobs PR #11328

Merged
merged 10 commits into from
Aug 22, 2024

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Aug 21, 2024

Ran into a few things I wanted to fix when I was reviewing the Jobs PR. This PR addresses most of those things. A few more are left as review comments on the Jobs PR itself

@Tobbe Tobbe added the release:chore This PR is a chore (means nothing for users) label Aug 21, 2024
@Tobbe Tobbe added this to the next-release milestone Aug 21, 2024

import { getPaths } from '@redwoodjs/project-config'

// This plugin is responsible for injecting the import path and name of a job
// into the object that is passed to createJob. This is later used by adapters
// and workers to import the job.

export default function ({ types: _t }: { types: typeof types }): PluginObj {
Copy link
Member Author

@Tobbe Tobbe Aug 21, 2024

Choose a reason for hiding this comment

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

Following our code standards of only using underscore (_foo) for unused variables

@@ -95,7 +95,7 @@ const tasks = async ({ force }) => {
fs.mkdirSync(getPaths().api.jobs)
} catch (e) {
// ignore directory already existing
if (!e.message.match('file already exists')) {
if (!/file already exists/.test(e.message)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@cannikin cannikin Aug 21, 2024

Choose a reason for hiding this comment

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

Yeah I hate this syntax, it feels backwards to me. In real life I'd say "Is this car red?" Not "Is the color red what this car is?" GROSS If you start with a regex object (because it was passed in as an argument, or you constructed the regex from other variables) you can use test(), but if you start with the string you can use match().

But if it's more performant or whatever, we can make it test() instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't argue with that... I also think it reads backwards 🙁 But it is the recommended way!
BUT I changed the code to pass { recursive: true }, which makes it so that no error is thrown if the directory already exists. So now we don't have to worry about any error messages 🎉

const workerArgs: string[] = []
workerArgs.push('--index', index.toString())
workerArgs.push('--id', id.toString())
const workerArgs = ['--index', index.toString(), '--id', id.toString()]
Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly wanted to get rid of the explicit string[] type annotation

@@ -16,8 +16,8 @@ import { prepareForRollback } from '../../../lib/rollback'
import { yargsDefaults } from '../helpers'
import { validateName, templateForComponentFile } from '../helpers'

// Makes sure the name ends up looking like: `WelcomeNotice` even if the user
// called it `welcome-notice` or `welcomeNoticeJob` or anything else
Copy link
Member Author

Choose a reason for hiding this comment

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

I just thought "anything else" was maybe a little too over reaching 😆

@Tobbe Tobbe added the changesets-ok Override the changesets check label Aug 21, 2024
@Tobbe Tobbe marked this pull request as ready for review August 21, 2024 19:30
@Tobbe Tobbe merged commit 19fa40a into redwoodjs:main Aug 22, 2024
47 checks passed
@Tobbe Tobbe deleted the tobbe-jobs-pr-review branch August 22, 2024 17:27
@Josh-Walker-GM Josh-Walker-GM modified the milestones: next-release, v8.0.0 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesets-ok Override the changesets check release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants