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

Noviny/update gatsby source workspaces #35

Merged
merged 4 commits into from
Nov 21, 2019

Conversation

Noviny
Copy link
Contributor

@Noviny Noviny commented Nov 21, 2019

No description provided.

@Noviny Noviny force-pushed the noviny/update-gatsby-source-workspaces branch from 26cb770 to a6d9caf Compare November 21, 2019 04:20
version: String
dir: String
relativeDir: String
packageJSON: JSON!

Choose a reason for hiding this comment

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

❤️

Copy link

@sarahbethfederman sarahbethfederman left a comment

Choose a reason for hiding this comment

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

Do we need commands for test-gatsby added in a package somewhere?
Otherwise, just questions and nitpicks. LGTM

extraFields: [
{
name: "readme",
definition: `String`,

Choose a reason for hiding this comment

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

why definition here and not type? Where are these types coming from? The graphql spec? Gatsby? TS?

Choose a reason for hiding this comment

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

@@ -1,4 +1,6 @@
const findWorkspacesRoot = require("find-workspaces-root").default;
const fs = require("fs-extra");
const path = require("path");

// Funfact, all gatsby code and example code uses template literlas instead of
// strings - see if you can figure out the places I copy/pasted code

Choose a reason for hiding this comment

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

Does this comment serve a purpose? 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entertainment value?

@Noviny
Copy link
Contributor Author

Noviny commented Nov 21, 2019

Do we need commands for test-gatsby added in a package somewhere?

There's a script to start it in develop mode, but maybe we need more things?

It's worth calling out that this package's construction has quite a bit of fast + loose about it, such as:

  • No tests
  • No types
  • very little previous code review

I haven't chose to address these things here, or look at other repo-level concerns. It's possible this is important enough that we want it as a ticket?

@Noviny Noviny merged commit 41dc2df into master Nov 21, 2019
@Noviny Noviny deleted the noviny/update-gatsby-source-workspaces branch November 21, 2019 23:10
@github-actions github-actions bot mentioned this pull request Nov 21, 2019
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