Minifier Windows support

[9larsons]

Resolves #14278.

Change Minifier to use node-glob instead of tiny-glob for file matching, as node-glob has Windows support. There was no apparent performance hit with this change.

[ErisDS]

Hey @9larsons thanks for the PR 🙂 As it's your first I had to hit the approve button before CI ran, and as you can see there's a lint error and some unit tests are failing.

Would you be able to take a look at resolving these issues?

[9larsons]

Hm, odd. I ran those both locally and had no issues before submitting. I'll definitely take a look.

[9larsons]

It seems Github's environment results in different results. Running my branch locally, I have passes for both:

Lint within Github is catching the missing semicolon while locally it is not. Any idea why the linter results would differ?

The other question is less clear... It seems the glob path is pulling the full path instead of the relative for the Minifier test failures in the Github tests. Github is running 'yarn workspaces run test:unit' vs. the developer instructions (and provided scripts) of just 'yarn test:unit'.

It seems both of these are the result of using workspaces. What should I be running to be correct?

[ErisDS]

The commands are correct, the issue here is with where you are running them.

Ghost is a monorepo with multiple packages inside /ghost. You're running commands inside ghost/core which is the main package / where the bulk of the application is, but running a yarn command here is scoped to that package.

The changes you've made live in ghost/minifier. Running the commands in that package will scope them again.

If you run the commands from the top level, they'll run across all packages using workspaces.

[9larsons]

Right, that makes sense. I guess I'm still thinking of the old client/server split.

I did initially try from the top level and was only able to make yarn test work when within the core package. Basically, I'm running in to a fair number of issues on Windows and I'm over time sorting them out... I will say, if you do any of this strictly on Linux, it all works great. There's ~17 failing Windows unit tests within ghost/core and I also cannot run yarn test from the parent/monorepo dir as there's a failure with another error.

I've started looking at resolving some of those failings Windows tests, but it seems like it's more trouble than just that.

At any rate, I'll get this resolved when I can. Thanks for the info.

[ErisDS]

For the purposes of these changes, you only need to get it working in ghost/minifier - but you'll prob need to test on windows + linux (even if that's just using CI) to make sure the tests are robust across OSes.

Doesn't windows have linux built in now anyway for this exact reason? 😬

[9larsons]

Yep, I did check on both Windows & Linux since it does need to work on both, but I wasn't previously at the right level for the tests to capture ghost/minifier as you mentioned.

Windows still makes you install Windows Subsystem for Linux. I've recently switched to that over an Ubuntu VM and it seems to work fairly well.

Frankly, I'm ready to scrap ever doing dev work on Windows 🙃.

[9larsons]

Sorry for the delay, busy week. It should be good now. I actually found a minifier unit test that didn't appear to be checking what it was intending to, and updated that. I think the two main callouts are:

1. node-glob always returns a Unix filepath, so I had to manually handle that and called it out in the card-assets tests as path.join would give incorrect results for Windows users.

2. I made the decision to pare back the error handling in minifier to no longer require a return value. It's debatable that you'd want to throw an error when given a glob or filepath that yields no file(s). I opted to not because the theme engine by default passes *.js and *.css to minifier, and if for some reason a theme did not have one of those, it seems odd to error when we're just trying to be helpful in collecting all files.

[codecov]

Codecov Report

Base: 67.27% // Head: 67.27% // No change to project coverage 👍

Coverage data is based on head (62cd52f) compared to base (62cd52f).
Patch has no changes to coverable lines.

❗ Current head 62cd52f differs from pull request most recent head 95154eb. Consider uploading reports for the commit 95154eb to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #15807   +/-   ##
=======================================
  Coverage   67.27%   67.27%           
=======================================
  Files        1625     1625           
  Lines      106022   106022           
  Branches    14974    14974           
=======================================
  Hits        71330    71330           
  Misses      33412    33412           
  Partials     1280     1280           

Flag	Coverage Δ
e2e-tests	76.59% <0.00%> (ø)
unit-tests	53.50% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[9larsons]

No clue off the bat why the post test would be failing, as this didn't touch that code.

I'm planning to get the MacBook set up tonight and finally put this to rest. I was working on this still using Win10 and WSL in parallel, and I'm learning to distrust WSL.

[ErisDS]

I've rebased this using github's tools (locally git rebase main and to pull it down git pull -r) and I've restarted the tests as this looks like a random failure to me.

[ErisDS]

I know you said you didn't see a perf impact, but I am still slightly nervous about this change. As far as I know the change to add this feature reduced boot performance significantly.

Tiny-glob claims to be a lot faster than node-glob but then it also claims to be have windows support (which is interesting).

Anyway all things considered I'd suggest we at least keep this async so that it doesn't block the boot process:

isaacs/node-glob#420 (comment)

[9larsons]

I've been out most of the past week. I'll take another pass at this here in the next few days.

I previously did some local testing with tiny-glob and wasn't getting any of the expected output. Both packages make the same claim about specifying globs with a forward slash regardless of platform, though only node-glob had a successful output for me on Windows. terkelg/tiny-glob#40 may have something to do with it.

When you say 'the change to add this feature' are you talking about adding the minifier? Or did you do some testing with this branch and looked at boot times? From what I saw, even if it is ~350% faster as it claims, it's all in the sub-millisecond territory.

[ErisDS]

By "the change to add this feature" I mean when I originally implemented the card asset pipeline. We have metrics for boot-time on Pro and they were noticeably impacted.

[9larsons]

Sorry, I synced my branch and it automatically closed this off. I'll open a new PR as I'm not sure if reopening this or opening a new one is easier for you. At any rate, after more investigation I found a solution we can both be happier with.