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
Metalsmith debug emit to files #13164
Conversation
...and reverse the turnary, because that's how it should be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESLint is disabled
vets-website
uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.
What you can do
See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.
@@ -0,0 +1,22 @@ | |||
/* eslint-disable no-console */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESLint disabled here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine, the only possible suggestion I have is that the naming of the plugin or the log output might not make it obvious that this step doesn't really change anything. If someone is looking at the build script, there are dozens of other plugins which are doing something - I just wouldn't want this to cause any confusion. However, if we plan on removing this in a month or so, I guess that isn't really a concern. 🤷
smith.use( | ||
emitToFiles( | ||
BUILD_OPTIONS, | ||
path.resolve(__dirname, '../../../../build/', 'after-layouts'), | ||
file => file.contents, | ||
), | ||
'Emit contents of files to build/after-layouts', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could get lost in the noise of the dozens of other build steps, and someone new might get confused trying to understand how these files get used or why we care about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's true. If I merge this into master
, I'll likely remove all uses of the plugin from the build script in favor of a single commented example since this is only to aid in debugging. I'll also add proper documentation on the function. 👍
For now, this was intended to help @drorva understand how the process works by seeing the output at various stages in the Metalsmith pipeline.
@@ -158,6 +167,15 @@ function build(BUILD_OPTIONS) { | |||
// translating .md files which would allow inPlace() and markdown() to be moved under the | |||
// permalinks() and navigation() filters making the variable stores uniform between inPlace() | |||
// and layout(). | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The only alternative I thought of is patching our Metalsmith wrapper (affectionately dubbed "Silversmith" because it's a shiny metal) to output all the files after all the steps, but since there are thousands of files and dozens of steps, that didn't seem like a great approach. (See screenshot below.) Instead, I opted to be deliberate about where we use the plugin.
If you have other suggested approaches, I'm all ears! Er...eyes, as the case may be. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Giving it more thought, actually, we could introduce another optional parameter to the smith.use()
command to output the files if --generate-files
is true. That would work essentially the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly, I think it's a matter of whether we want to keep this functionality of emitting files in the middle of the pipeline for debugging purposes. If this is a thing we want to keep, it'll need some more polishing. Like better handling of how to specify the output directory, separating those directories per build type, but keeping them distinct from all "real" build outputs, etc. As it stands right now, this is something I whipped up in like 30 minutes to get some examples I could use to show how the build process worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pause on this while we see how the our other effort is progressing.
Description
Using the
emitToFiles
plugin, we can dump the contents of the files to disk at various points of the build process. This will aid in debugging.Enable this functionality with
yarn build:content --generate-files
Testing done
Tested manually.
Screenshots
Not really applicable, but I did add it to the
yarn build:content help
command: