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.

Sign up for GitHub

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

Jump to bottom

feat(gatsby): Remove deleteNode with ID arg #29205

Merged
merged 10 commits into from
Feb 10, 2021
Merged

feat(gatsby): Remove deleteNode with ID arg #29205

merged 10 commits into from
Feb 10, 2021

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Jan 26, 2021 •
edited
Loading

Description

This removes the possibility to use deleteNode with the ID of the node. This was deprecated in favor of passing in the whole node and now will be removed.

New deprecation

We align createNode and deleteNode and add a new deprecation notice to deleteNode.
So that at the end it will be createNode(node) and deleteNode(node) instead of deleteNode({ node })

Documentation

The docs (source plugin guide) are updated.

Related Issues

[ch23642]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 26, 2021
@LekoArts LekoArts added topic: internal and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 26, 2021
@wardpeet
Copy link
Contributor

Should we align createNode and deleteNode and add a new deprecation notice to deleteNode?
So that at the end it will be createNode(node) and deleteNode(node) instead of deleteNode({ node })

@LekoArts I would say yes

@LekoArts LekoArts mentioned this pull request Jan 28, 2021
Merged
@LekoArts LekoArts added the topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label label Jan 29, 2021
@LekoArts LekoArts marked this pull request as ready for review February 2, 2021 10:05
@wardpeet wardpeet changed the title gatsby: Remove deleteNode with ID arg feat(gatsby): Remove deleteNode with ID arg Feb 10, 2021
@LekoArts
Copy link
Contributor Author

@TylerBarnes To get myself unblocked and you not urged to merge your tests ASAP I reverted the WordPress changes :) The current state is now using deprecated APIs then, but it still works.

laurieontech
laurieontech reviewed Feb 10, 2021
View reviewed changes
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Left a minor comment on the error message, but it's non-blocking and just a suggestion.

I did notice that this typescript definition isn't updated as part of the PR.

gatsby/packages/gatsby/index.d.ts

Line 1130 in e691342

deleteNode(

I assume that's generated?

packages/gatsby/src/db/__tests__/nodes.js Outdated
)
expect(getNode(`hi`)).toBeUndefined()
const deprecationNotice =
`Calling "deleteNode" with a nodeId is deprecated. Please pass an ` +
`object containing a full node instead: deleteNode({ node }). ` +
`Calling "deleteNode" with an object containing a full node is deprecated. Please pass ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

I might change this slightly?

"Calling 'deleteNode' with {node} is deprecated. Please pass the node directly to the function: deleteNode(node)."

An object containing the full node reads a bit confusing in my mind.

packages/gatsby/src/redux/actions/public.js Outdated
if (args && args.name) {
// `plugin` used to be the third argument
plugin = args
`Calling "deleteNode" with an object containing a full node is deprecated. Please pass ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

@LekoArts
Copy link
Contributor Author

It is updated? :)

https://github.com/gatsbyjs/gatsby/pull/29205/files#diff-15724d4352562f842e4eb9df3a864711b1617ea4ebabc56c2aee4e975bca6767

@laurieontech
Copy link
Contributor

No idea how I missed that 😅

laurieontech
laurieontech approved these changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@LekoArts LekoArts merged commit 01b6123 into master Feb 10, 2021
@LekoArts LekoArts deleted the v3/delete-node-with-id-argument branch February 10, 2021 16:32
This was referenced Mar 12, 2021
Closed
Merged
Merged
Closed
Closed
This was referenced Mar 17, 2021
Closed
Closed
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Closed
Closed
@LekoArts LekoArts mentioned this pull request Sep 22, 2021
Merged
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
@LekoArts
feat(gatsby): Remove deleteNode with ID arg (gatsbyjs#29205)
15f1dfd 
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurieontech laurieontech laurieontech approved these changes

Assignees
No one assigned
Labels
topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LekoArts @wardpeet @laurieontech