fix(themes): fix a template identifier matching issue on Windows

luis-almeida commented 1 year ago

Duplicate of https://github.com/zendesk/zcli/pull/200 in an attempt to fix the build (possibly due to the branch name) @moremada I'm keeping your commits so hoping this could be an ok solution if it works.

Description

Fixes an issue on Windows where the template identifier incorrectly matches due to non-posix path separator being used, but posix path separators being expected.

`bdcd772` fix: enforce posix separator in glob result to match split pattern

`e58a604` fix: bump glob version

glob posix option is only available starting from 10.1.0

`98ce466` fix: update yarn.lock

Detail

On Windows, the globSync function returns the \\ path separator by default. However, we are matching against a posix path separator of / as can be seen on line 9's split pattern of template.split('templates/'). This is causing the following error:

TypeError: Cannot read properties of undefined (reading 'split')

This PR explicitly instructs glob to return path results using posix path separators and allows the split pattern to match correctly.

Checklist

[ ] :guardsman: includes new unit and functional tests

moremada commented 1 year ago

@luis-almeida I ran into another related issue. In fact, the { posix: true } fix in this PR causes it!

Description

On Windows, when running zcli themes:preview ./themeFolder the Help Center returns Service Unavailable (503 error) upon visiting the preview page.

Specifically, if a theme path is provided, things break. Alternatively, if I cd into the theme folder and run zcli themes:preview, then all works well.

It appears that when a theme path is not provided, and defaults to ., then on Windows, the glob function just uses relative paths, meaning there are no Windows backslashes. But when a theme path differs from the cwd, then it uses absolute paths, which include the Windows backslashes...

Issue

Ultimately, we are dealing with another Windows path separator issue.

When we use the glob { posix: true } option, it not only outputs the paths with /, but it also expects the paths to have /.

Fix

I believe the full fix is instead, this:

const filenames = (0, glob_1.globSync)(`${themePath.replace(/\\/g, '/')}/templates/**/*.hbs`, { posix: true });

We need to change the Windows backslashes to forward slashes to accommodate the case in which glob decides to use absolute paths and include non-posix path separators.

moremada commented 1 year ago

@luis-almeida Thank you for all the help. Have you had a chance to take a look at my latest comment? 🙏

luis-almeida commented 8 months ago

Closing in favour of https://github.com/zendesk/zcli/pull/220 which includes the commit in this PR and just got released. @moremada your comment was addressed there as well.

zendesk / zcli