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(gatsby): convert sanitize-node to typescript #36327

Merged
merged 7 commits into from
Aug 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/gatsby/src/redux/actions/public.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const {
const { splitComponentPath } = require(`gatsby-core-utils/parse-component-path`)
const { hasNodeChanged } = require(`../../utils/nodes`)
const { getNode, getDataStore } = require(`../../datastore`)
const sanitizeNode = require(`../../utils/sanitize-node`)
import sanitizeNode from "../../utils/sanitize-node"
const { store } = require(`../index`)
const { validatePageComponent } = require(`../../utils/validate-page-component`)
import { nodeSchema } from "../../joi-schemas/joi"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,25 @@
const sanitizeNode = require(`../sanitize-node`)
import sanitizeNode from "../sanitize-node"

describe(`node sanitization`, () => {
let testNode

beforeEach(() => {
const circularReference = {}
// @ts-ignore edge test case
circularReference.self = circularReference
const indirectCircular = {
down1: {
down2: {},
},
}
// @ts-ignore edge test case
indirectCircular.down1.down2.deepCircular = indirectCircular

testNode = {
id: `id1`,
parent: null,
children: [],
unsupported: () => {},
unsupported: (): void => {},
inlineObject: {
field: `fieldOfFirstNode`,
re: /re/,
Expand All @@ -26,7 +28,7 @@ describe(`node sanitization`, () => {
repeat2: `bar`,
repeat3: {
repeat3: {
test: () => {},
test: (): void => {},
repeat: `bar`,
},
},
Expand All @@ -46,8 +48,11 @@ describe(`node sanitization`, () => {
it(`Remove not supported fields / values`, () => {
const result = sanitizeNode(testNode)
expect(result).toMatchSnapshot()
// @ts-ignore test properties that shouldn't be there are not there
expect(result.unsupported).not.toBeDefined()
// @ts-ignore test properties that shouldn't be there are not there
expect(result.inlineObject.re).not.toBeDefined()
// @ts-ignore test properties that shouldn't be there are not there
expect(result.inlineArray[3]).not.toBeDefined()
})

Expand All @@ -66,7 +71,7 @@ describe(`node sanitization`, () => {
it(`Doesn't create clones if it doesn't have to`, () => {
const testNodeWithoutUnserializableData = {
id: `id1`,
parent: null,
parent: ``,
children: [],
inlineObject: {
field: `fieldOfFirstNode`,
Expand All @@ -76,7 +81,9 @@ describe(`node sanitization`, () => {
type: `Test`,
contentDigest: `digest1`,
owner: `test`,
counter: 0,
},
fields: [],
}

const result = sanitizeNode(testNodeWithoutUnserializableData)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,60 @@
import _ from "lodash"

import type { IGatsbyNode } from "../redux/types"
import type { GatsbyIterable } from "../datastore/common/iterable"

type data = IGatsbyNode | GatsbyIterable<IGatsbyNode>

/**
* @param {Object|Array} data
* @returns {Object|Array} data without undefined values
*/
type omitUndefined = (data: data) => Partial<data>

const omitUndefined: omitUndefined = data => {
const isPlainObject = _.isPlainObject(data)
if (isPlainObject) {
return _.pickBy(data, p => p !== undefined)
}

return (data as GatsbyIterable<IGatsbyNode>).filter(p => p !== undefined)
}

/**
* @param {*} data
* @return {boolean}
*/
type isTypeSupported = (data: data) => boolean

const isTypeSupported: isTypeSupported = data => {
if (data === null) {
return true
}

const type = typeof data
const isSupported =
type === `number` ||
type === `string` ||
type === `boolean` ||
data instanceof Date

return isSupported
}

/**
* Make data serializable
* @param {(Object|Array)} data to sanitize
* @param {boolean} isNode = true
* @param {Set<string>} path = new Set
*/
const sanitizeNode = (data, isNode = true, path = new Set()) => {

type sanitizeNode = (
data: data,
isNode?: boolean,
path?: Set<unknown>
) => data | undefined

const sanitizeNode: sanitizeNode = (data, isNode = true, path = new Set()) => {
const isPlainObject = _.isPlainObject(data)

if (isPlainObject || _.isArray(data)) {
Expand All @@ -20,15 +68,15 @@ const sanitizeNode = (data, isNode = true, path = new Set()) => {
returnData[key] = o
return
}
returnData[key] = sanitizeNode(o, false, path)
returnData[key] = sanitizeNode(o as data, false, path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this loop to a Object.entries(data).forEach? That should allow TS to pick up the right types and we shouldn't need to cast.

Copy link
Contributor Author

@Kornil Kornil Aug 9, 2022

Choose a reason for hiding this comment

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

Object.entries hardcodes the first value o as string, making tests fail (Symbol/string issue, .each avoids this issue).


if (returnData[key] !== o) {
anyFieldChanged = true
}
})

if (anyFieldChanged) {
data = omitUndefined(returnData)
data = omitUndefined(returnData as data) as data
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can type returnData during initialization, we should be able to get away with no casting here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

returnData is initalised as {} or [].

  • {} is not a valid IGatsbyNode error: Type '{}' is missing the following properties from type 'IGatsbyNode': id, parent, children, internal, fields
  • [] is not a valid GatsbyIterable<IGatsbyNode> error:
    Type 'never[]' is missing the following properties from type 'GatsbyIterable': source, deduplicate, mergeSorted, intersectSorted, deduplicateSorted

I could cast when we define returnData to avoid the double casting here, but we would still need one as omitUndefined returns a Partial<data> (from lodash, pickBy returns a Partial).

I think it's down to personal preference here, I feel the double cast here is more significant than having it when we define returnData but I'm open to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable enough. Thank you for the detailed response. 😄

}

// arrays and plain objects are supported - no need to to sanitize
Expand All @@ -42,36 +90,4 @@ const sanitizeNode = (data, isNode = true, path = new Set()) => {
}
}

/**
* @param {Object|Array} data
* @returns {Object|Array} data without undefined values
*/
const omitUndefined = data => {
const isPlainObject = _.isPlainObject(data)
if (isPlainObject) {
return _.pickBy(data, p => p !== undefined)
}

return data.filter(p => p !== undefined)
}

/**
* @param {*} data
* @return {boolean}
*/
const isTypeSupported = data => {
if (data === null) {
return true
}

const type = typeof data
const isSupported =
type === `number` ||
type === `string` ||
type === `boolean` ||
data instanceof Date

return isSupported
}

module.exports = sanitizeNode
export default sanitizeNode