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

HTML File Path Support #32

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Conversation

TurtIeSocks
Copy link
Contributor

  • Add a string path to an HTML template file instead of a direct template

Resolves #6

- Add a string path to an HTML template file instead of a direct template

Resolves craftamap#6
Copy link
Owner

@craftamap craftamap left a comment

Choose a reason for hiding this comment

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

Uh nice, thanks for taking care of the issue.
Left some small comments. :^)

src/index.ts Outdated
Comment on lines 126 to 127
const compiledTemplateFn = htmlTemplate.toString().endsWith('.html')
? lodashTemplate(fs.readFileSync(htmlTemplate).toString())
Copy link
Owner

Choose a reason for hiding this comment

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

Let us also check if the file exists using fs.exists.

I also think some people are still calling their html files just .htm.

src/index.ts Outdated
Comment on lines 126 to 127
const compiledTemplateFn = htmlTemplate.toString().endsWith('.html')
? lodashTemplate(fs.readFileSync(htmlTemplate).toString())
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can safely convert renderTemplate to be async, and use await fs.readFile from the promises package instead of renaming all the references of fs to promises.

@TurtIeSocks
Copy link
Contributor Author

In regards to both comments, .exists is not available on the fs.promises api, so would you prefer to do the check with the non-promise API, which would mean keeping the renaming, or to wrap something like fs.promises.stats in a try/catch block to determine whether the file exists but still using promises?

@craftamap
Copy link
Owner

Maybe the smartest thing would be to switch the whole import to

import fs from 'fs'

and use fs.promises.<method> everywhere we need the promises api.

According to the docs, fs.exists is deprecated, but fs.existsSync is not, so maybe we could use this?

- Rework renderTemplate function a bit
- Check if htmlTemplate prop exists, if it does and points to a valid file, read that file
- If it doesn't read a valid file, attempt to load the raw html string, and if that's invalid, load the default html fallback
- Add a logging function to indicate whether the path reads a valid file
@TurtIeSocks
Copy link
Contributor Author

Sounds good, let me know what you think of the latest commit.

src/index.ts Outdated
const template = (htmlTemplate && fs.existsSync(htmlTemplate)
? await fs.promises.readFile(htmlTemplate)
: htmlTemplate || '').toString()
const isValid = template.startsWith('<!DOCTYPE html>')
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like this is a bit too error prone. For example, the templates in the README don't work with this anymore. Can we leave this out for now? We can create a issue for checking the validity of a document in the future. :)

Copy link
Owner

@craftamap craftamap 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 now! I'm unsure about the doctype check, let's add a issue for this.

Thanks for your help on this issue, it's really appreciated!

@TurtIeSocks
Copy link
Contributor Author

Np, error check removed in latest commit.

@craftamap
Copy link
Owner

Let's go, will be released within the next days :)

@craftamap craftamap merged commit af412bc into craftamap:master Apr 21, 2022
@TurtIeSocks TurtIeSocks deleted the html-template-path branch April 21, 2022 19:56
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.

Add feature to provide a htmlTemplatePath instead of a htmlTemplate directly
2 participants