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

fix: adds support for builds with cache #10

Merged
merged 7 commits into from
Dec 4, 2021
Merged

Conversation

gdorsi
Copy link
Contributor

@gdorsi gdorsi commented Dec 2, 2021

fixes #3

@gdorsi gdorsi marked this pull request as ready for review December 2, 2021 18:20
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@gdorsi gdorsi requested a review from mcollina December 3, 2021 08:17
@@ -13,7 +13,9 @@ test('it should correctly generated all required pino files', (t) => {
const distFolder = resolve(__dirname, '../tmp/dist')

t.teardown(() => {
spawnSync(`rm -rf ${distFolder}`)
spawnSync(`rm -rf ${distFolder}`, {
shell: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because the command didn't locate correctly the rm executable so the tmp dir was not deleted.

)
}

test('it should correctly generated all required pino files', (t) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
test('it should correctly generated all required pino files', (t) => {
test('it should correctly generate all required pino files', (t) => {

Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this testing that the output is being cached? It looks like it's testing that the output is generated correctly the second time you run the build, but not that the cache is being used. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The caching is enabled in the webpack configuration and I'm testing that nothing breaks when it is enabled.

I've renamed the test to make the intent more clear.

})
})

runBuild(distFolder, () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a check that this doesn't error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mcollina mcollina merged commit 6a40e72 into pinojs:main Dec 4, 2021
@github-actions github-actions bot mentioned this pull request Dec 5, 2021
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.

Plugin is not working with webpack cache
3 participants