Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Add toFormattedDataForProps unit test #139

Merged

Conversation

paulshryock
Copy link
Contributor

@paulshryock paulshryock commented Jan 17, 2022

Summary

This adds a passing unit test for toFormattedDataForProps to confirm that output eleventyData is formatted correctly. It also adds some sensible fallback values to the function.

Testing

  • npm run build:slinkity
  • npm test -- --coverage
  • npm run lint

Ticket

Unit test core functionality #21

Screenshot

Screenshot of successful build results, passing unit tests, and successful linting with no errors

@netlify
Copy link

netlify bot commented Jan 17, 2022

✔️ Deploy Preview for slinkity canceled.

🔨 Explore the source changes: f99af95

🔍 Inspect the deploy log: https://app.netlify.com/sites/slinkity/deploys/61e59e6268f37100081845fa

@paulshryock
Copy link
Contributor Author

Found an issue in this... making another quick change.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the pull request! So to be honest: I'd rather avoid adding unit tests for type checks to this repo. We use JSDocs (which should be linted! #97) to enforce types right now. If we introduce a testing pattern for types, I could see it spiraling into countless other unit tests I'd rather avoid writing and maintaining.

I'm also not a fan of adding thorough type checking to the toFormattedDataForProps function. I could see this dramatically slowing down page generation at scale; the type check on each nested collection alone is a lot of work. For type safety, I'd prefer assigning default values for nonexistent keys like so:

for (let name of Object.keys(eleventyData.collections ?? {})) {
    formattedCollections[name] = eleventyData.collections[name]?.map((item) => {
      // eslint-disable-next-line no-unused-vars
      const { collections, ...data } = item.data
      return {
        inputPath: item.inputPath ?? '',
        fileSlug: item.fileSlug ?? '',
        filePathStem: item.filePathStem ?? '',
        date: item.date ?? '',
        outputPath: item.outputPath ?? '',
        url: item.url ?? '',
        data,
        get templateContent() {
          return item.templateContent
        },
      }
    })
  }

This ensures that our collections object doesn't deviate too far from 11ty's collections object. Nonexistent keys won't break standard 11ty builds, so they shouldn't break ours either!

So if you want to add type checks, I'd suggest:

  • trading out the type checks for default assignment
  • removing the type check unit test, in favor of a unit test on the expected output of the toFormattedDataForProps function

Open to discussion, but that's my 2 cents!

@paulshryock
Copy link
Contributor Author

@Holben888, yeah I much prefer that approach as well. I'll make those changes.

@paulshryock paulshryock changed the title [#21] Add toFormattedDataForProps arg validation and unit test [#21] Add toFormattedDataForProps unit tests (in progress) Jan 17, 2022
@paulshryock
Copy link
Contributor Author

Closing this for now, and I'll re-open later with the changes discussed above.

@paulshryock paulshryock reopened this Jan 17, 2022
@paulshryock paulshryock changed the title [#21] Add toFormattedDataForProps unit tests (in progress) [#21] Add toFormattedDataForProps unit tests Jan 17, 2022
@paulshryock
Copy link
Contributor Author

@Holben888 this is ready for review.

@paulshryock paulshryock changed the title [#21] Add toFormattedDataForProps unit tests Add toFormattedDataForProps unit test Jan 17, 2022
@ghost ghost merged commit 1a97dc9 into slinkity:main Jan 18, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant