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(gatsby-source-drupal): added preview feature #14630

Merged
merged 19 commits into from Jul 8, 2019

Conversation

@grantglidewell
Copy link
Contributor

commented Jun 7, 2019

Description

Drupal source plugin Preview feature!

related Drupal module: https://www.drupal.org/project/gatsby

@grantglidewell grantglidewell requested a review from gatsbyjs/core as a code owner Jun 7, 2019

@grantglidewell

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

I tested the upstream changes in the separate npm package gatsby-source-drupal-preview and it appears to be working. The caveat on existing projects is that you will need to kill node-modules and cache before a rebuild will work.

grantglidewell and others added some commits Jun 11, 2019

packages/gatsby-source-drupal/src/gatsby-node.js Outdated Show resolved Hide resolved
@@ -264,4 +267,105 @@ exports.sourceNodes = async (
for (const node of nodes) {
createNode(node)
}

if (process.env.NODE_ENV === `development` && preview) {

This comment has been minimized.

Copy link
@KyleAMathews

KyleAMathews Jun 12, 2019

Contributor

Curious if there's any way to unify logic between here & the regular fetches?

This comment has been minimized.

Copy link
@grantglidewell

grantglidewell Jun 13, 2019

Author Contributor

could you elaborate?

@@ -264,4 +267,105 @@ exports.sourceNodes = async (
for (const node of nodes) {
createNode(node)
}

if (process.env.NODE_ENV === `development` && preview) {
const server = micro(async (req, res) => {

This comment has been minimized.

Copy link
@pieh

pieh Jun 12, 2019

Contributor

Could you elaborate why do we need to create separate server?

This comment has been minimized.

Copy link
@grantglidewell

grantglidewell Jun 13, 2019

Author Contributor

There is no way to access the express instance in this method, if it were exposed I would use it. I need access to the initial data pulled from Drupal and use of actions and user params so putting all of this functionality in the onCreateDevServer export wasnt a viable option.

This comment has been minimized.

Copy link
@pieh

pieh Jun 13, 2019

Contributor

Gotcha. I'll think about way to not have to do this.

This introduces some potential issues - 8080 port is hardcoded, so there would be crashes when you have 2 drupal sources (or just trying to run 2 separate development servers with it). And instead of trying to address that it would be great to remove the need for spawning extra server

This comment has been minimized.

Copy link
@pieh

pieh Jun 13, 2019

Contributor

need access to the initial data pulled from Drupal and use of actions and user params

actions and user params are available in every API:

exports.onCreateDevServer = (
  { app, createNodeId, actions, store, cache, createContentDigest },
  { baseUrl, basicAuth }
) => {

Will work.

Checking more on this - seems like thing that are referenced are backRefs/addBackRef (that doesn't allow easily moving it as-is). I do think we can address this. I'll try to do some work on this before weekend.

This comment has been minimized.

Copy link
@grantglidewell

grantglidewell Jun 13, 2019

Author Contributor

I totally agree, I know there would be potential issues with exposing it to all source plugins, I believe @DSchau mentioned something about dragons 🐲

grantglidewell added some commits Jun 13, 2019

}

node.internal.contentDigest = createContentDigest(node)
createNode(node)

This comment has been minimized.

Copy link
@pieh

pieh Jun 13, 2019

Contributor

Here we are updating/creating just node that represents just changed "entry". Because of back reference fields we might need to update more nodes (all other nodes that reference this updated node). It might be tricky to do correctly, because we not only have to cover cases where we add back reference to other nodes - we also might need to remove those.

This comment has been minimized.

Copy link
@grantglidewell

grantglidewell Jun 13, 2019

Author Contributor

true, because of the way we have the Drupal plugin working, the dependent nodes are updated and then the final parent node is updated (the nodes to update are sent in a specific order). I haven't run into a problem with these getting out of sync, it works with paragraphs style data which seems to use this back dependency. There are definitely possible issues with this but I haven't found any that don't work correctly yet.

This comment has been minimized.

Copy link
@grantglidewell

grantglidewell Jun 13, 2019

Author Contributor

I'm not sure if that's totally clear, but all affected nodes from a change are posted to the endpoint and updated

This comment has been minimized.

Copy link
@pieh

pieh Jun 13, 2019

Contributor

Oh, so this is handled in Drupal module. Sorry for noise, I should have try debugging that.

This comment has been minimized.

Copy link
@grantglidewell

grantglidewell Jun 13, 2019

Author Contributor

I still think there are potential issues, but for now it works because we have that ordering implemented there.

This comment has been minimized.

Copy link
@pieh

pieh Jun 27, 2019

Contributor

I was looking a little at drupal module code and it doesn't seem like there is any batching happening there - https://git.drupalcode.org/project/gatsby/blob/8.x-1.x/gatsby.module - am I missing something?

I added handling for back reference updates, so this shouldn't be an issue - I just wanted to fact check on this.

This comment has been minimized.

Copy link
@smthomas

smthomas Jul 3, 2019

There is no batching currently happening. It currently sends all updates/inserts individually. This was the easiest way to get the proof of concept off the ground. If we can determine how we would like the data to be batched, we can add it to the roadmap for the Drupal module.

@pieh

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Did another review pass and few notes:

  • there is no handling of content deletion currently. Not a blocker for initial merge, but need to know if there will be breaking changes to webhook payload to support that case
  • there is some trickiness with backreferences - right now those get duplicated and changes are not reflected - I'm working right now on fixing this (and some refactoring to share more code from initial sync handling and webhook handling)

--edit:
Second item is handled now

@pieh pieh changed the title added preview feature to drupal source module feat(gatsby-source-drupal): added preview feature Jun 28, 2019

@pieh

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

All right, this should be good now:

  • removed micro (just using gatsby dev server now)
  • added handling of back references:
    • if updated node A no longer references other node (B) - that backreference get's removed
    • if updated node A has new reference - that backreference is added
  • added some basic tests for node updating and inserting
pluginOptions
) => {
app.use(
`/___updatePreview/`,

This comment has been minimized.

Copy link
@pieh

pieh Jun 28, 2019

Contributor

I would propose to use /___update/gatsby-source-drupal or something like this here, so the update endpoint is namespaced. (This would need changes in Gatsby drupal module)

This comment has been minimized.

Copy link
@smthomas

smthomas Jul 3, 2019

If you provide me with the namespaces I can update the Drupal module to reflect this once it's merged. I'm assuming you would want to use ___update, ___create and ___delete?

This comment has been minimized.

Copy link
@pieh

pieh Jul 8, 2019

Contributor

I don't have opinion on separate endpoints for update/create/delete operations - it can stay as single endpoint and operation could be send with payload (for our use case update and create will have same handler anyway - only delete will have different handling). Only thing that I'm interested in is using namespace for gatsby-source-drupal (as we will likely start adding similar endpoints for other plugins)

This comment has been minimized.

Copy link
@pieh

pieh Jul 8, 2019

Contributor

For the sake of time. I think it's fine to merge this - this PR was in limbo for too long. I'll just add explicit note to README that there likely will be breaking changes coming that will require updating both gatsby-source-drupal and gatsby drupal module in future

@pieh

pieh approved these changes Jul 8, 2019

Copy link
Contributor

left a comment

Let's ship it

@gatsbybot gatsbybot merged commit a045a32 into gatsbyjs:master Jul 8, 2019

18 checks passed

Danger All good
Details
Peril All green. Congrats.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsbygram Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: theme_starters_validate Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
unit_tests_windows Build #20190708.14 succeeded
Details
@pieh

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Published gatsby-source-drupal@3.2.3

rayriffy added a commit to rayriffy/gatsby that referenced this pull request Jul 14, 2019

feat(gatsby-source-drupal): added preview feature (gatsbyjs#14630)
* added preview feature to drupal source module

* Update README.md

* added a more robust check for relationship data, empty arrays were causing errors in preview updates

* Update README.md

* reverted backticks in comments

* refactor: don't spawn extra server, re-use node creation and relationship handling

* there is no more preview config option

* remove stray console.log

* fix tests

* mark as experimental

* move webhook update handling to separate function to make it easier to test

* add tests for updating function

* add more notes about experimental state of preview feature

* move utils to separate file - fixes exporting variable that is not gatsby API

* normalize newline in gitignore

johno added a commit to johno/gatsby that referenced this pull request Jul 17, 2019

feat(gatsby-source-drupal): added preview feature (gatsbyjs#14630)
* added preview feature to drupal source module

* Update README.md

* added a more robust check for relationship data, empty arrays were causing errors in preview updates

* Update README.md

* reverted backticks in comments

* refactor: don't spawn extra server, re-use node creation and relationship handling

* there is no more preview config option

* remove stray console.log

* fix tests

* mark as experimental

* move webhook update handling to separate function to make it easier to test

* add tests for updating function

* add more notes about experimental state of preview feature

* move utils to separate file - fixes exporting variable that is not gatsby API

* normalize newline in gitignore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.