fix: build on windows machines

[catamphetamine]

When I recently forked the repo, while attempting to run npm run build command in order to build the package, the build didn't run on my Windows machine.
The reasons turned out to be:

• fast-glob package not recognizing backslashes.
• The build script not specifying the executable to run the .mjs file with.

After applying the changes in this PR, the build process has finished without errors.

[WikiRik]

I'm not sure how often people build sequelize on Windows, but it might be nice to add it as a job to the CI so we can keep an eye on it

[AjaniBilby]

When I pulled this and ran yarn lerna run test-unit I got the following errors on Win11 Version 10.0.22621.1992

  2091 passing (969ms)
  6 pending
  3 failing

  1) importModels
       can import models using a single glob path:

      AssertionError: expected [] to have a length of 2 but got +0
      + expected - actual

      -0
      +2

      at Proxy.assertLength (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai\lib\chai\core\assertions.js:2144:10)
      at doAsserterAsyncAndAddThen (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai-as-promised\lib\chai-as-promised.js:289:22)
      at Proxy.<anonymous> (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai-as-promised\lib\chai-as-promised.js:272:28)
      at Proxy.overwritingChainableMethodWrapper (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai\lib\chai\utils\overwriteChainableMethod.js:60:34)
      at Proxy.chainableMethodWrapper (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai\lib\chai\utils\addChainableMethod.js:113:49)
      at Context.<anonymous> (test\unit\import-models\import-models.test.ts:13:28)

  2) importModels
       can import models using multiple glob paths:

      AssertionError: expected [] to have a length of 2 but got +0
      + expected - actual

      -0
      +2

      at Proxy.assertLength (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai\lib\chai\core\assertions.js:2144:10)
      at doAsserterAsyncAndAddThen (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai-as-promised\lib\chai-as-promised.js:289:22)
      at Proxy.<anonymous> (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai-as-promised\lib\chai-as-promised.js:272:28)
      at Proxy.overwritingChainableMethodWrapper (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai\lib\chai\utils\overwriteChainableMethod.js:60:34)
      at Proxy.chainableMethodWrapper (C:\Users\me\Documents\GitHub\sequelize\node_modules\chai\lib\chai\utils\addChainableMethod.js:113:49)
      at Context.<anonymous> (test\unit\import-models\import-models.test.ts:21:28)

  3) importModels
       can exclude results using the second parameter:
     TypeError: Cannot read properties of undefined (reading 'path')
      at Context.<anonymous> (test\unit\import-models\import-models.test.ts:37:21)






 ——————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Ran target test-unit for 2 projects and 2 tasks they depend on (13s)

    √    3/4 succeeded [0 read from cache]

    ×    1/4 targets failed, including the following:
         - @sequelize/core:test-unit

[catamphetamine]

@AjaniBilby You sure the errors are specific to the changes in this PR?
See if you could check out the latest main branch and then re-apply the changes from this PR to it, and then run it.

Also, I dunno why is this PR marked as "Draft". I don't seem to have created it that way. It's ready for merging. There doesn't seem to be a button to unmark it as such. If the "Draft" status is something intentional marked by the repo authors then it's fine.

[WikiRik]

It's not related to the changes in this PR, but we do not have passing tests on Windows with this change. So I think it would fall in the scope of this PR.

Same as to why it's in draft. Without a CI job on Windows we don't know if it remains working so that would in my opinion be something that needs to be added before we can merge.

That Windows CI job only needs passing tests on sqlite imo, not all dialects.

EDIT: it is also possible that you want the scope of this PR to be limited to only building on Windows. In that case we can open a future PR to actually let the tests pass and only have a Windows CI job for successful building on Windows

[catamphetamine]

@WikiRik Ah, I see, so a windows-specific test would be required to consider this PR complete?

I don't think that would be possible though. And even if it breaks Windows in any way, it's already broken as is, so my suggestion would be merging this PR as is rather than leaving it unmerged.

Or, an independent 3rd party could confirm that the changes in this PR fix their Windows run. For example, @AjaniBilby.

[catamphetamine]

@WikiRik I mean, having a Windows CI builder for free for an open-source project isn't something I'd imagine existing. Why not let the users be such a CI builder.

[WikiRik]

@WikiRik Ah, I see, so a windows-specific test would be required to consider this PR complete?

In my reply I started with a test, but I would also be fine with only a successful build on Windows. Then future PRs can add functionality that tests pass on Windows as well.

@AjaniBilby mentioned on Slack this morning that GitHub Actions has support for Windows so it should not be too difficult to implement. I haven't taken a look into it yet today

Edit; https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources
It's Windows Server, but that should have the same issues as Windows 10/11 I think

[catamphetamine]

@WikiRik I see. So this job:

sequelize/.github/workflows/ci.yml

Lines 16 to 23 in b2b9048

	jobs:
	install-and-build:
	strategy:
	fail-fast: false
	matrix:
	node-version: [16, 20]
	name: Upload install and build artifact (Node ${{ matrix.node-version }})
	runs-on: ubuntu-latest

Could just be copy-pasted while replacing runs-on: ubuntu-latest with runs-on: windows-latest with the job name changed from install-and-build to install-and-build-on-windows. Then, somehow, a new "checkmark" called "install-and-build-on-windows` would appear at the bottom of this PR and it will become green if it passes, or smth like that. That's as far as I've heard of GitHub CI. Haven't set it up myself yet.

[WikiRik]

Exactly! You can use these lines;

sequelize/.github/workflows/ci.yml

Lines 17 to 33 in b2b9048

	install-and-build:
	strategy:
	fail-fast: false
	matrix:
	node-version: [16, 20]
	name: Upload install and build artifact (Node ${{ matrix.node-version }})
	runs-on: ubuntu-latest
	steps:
	- uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
	- uses: actions/setup-node@e33196f7422957bea03ed53f6fbb155025ffc7b8 # v3.7.0
	with:
	node-version: ${{ matrix.node-version }}
	cache: yarn
	- name: Install dependencies and build sequelize
	run: yarn install --immutable
	- name: Build sequelize
	run: yarn build

The compressing of the artifact is not needed for building so you can skip that.

[AjaniBilby]

The new CI job for Windows testing appears to only check it builds successfully on windows unless I'm missunderstanding something.
I pulled the latest changes and definetly still fail on the same three glob path test cases.

Even if the glob path is just an issue with my environment rather than the project, the fact it can fail depending on environment seems like it is something then that should be tested in an automated environment.

Edit: Just to clarify I'm not exactly sure if the glob path error I'm still getting is actually due to this PR specifically, but I do know without this PR I couldn't even get to the point where I am getting the glob path.
This could be broken out into a second PR after this one is complete if we believe it is a seperate issue - since the issue is actually with the execution of the test suite on windows, rather than just purely running yarn build.

[WikiRik]

The new CI job for Windows testing appears to only check it builds successfully on windows unless I'm missunderstanding something.

Correct

I pulled the latest changes and definetly still fail on the same three glob path test cases.

These can be solved in a different PR. I don't think they have been caused by your changes

[WikiRik]

Okay, maybe I spoke too soon

[catamphetamine]

@WikiRik I dunno why it fails on the CI though. Runs fine on my machine.

[AjaniBilby]

I should have done more digging, the same fixes done in this commit should have been applied to package/core/test/unit/import-models/import-modes.test.ts then all test cases will pass.

Once I have solved the issue I'll make a new PR which will also include a CI job which tests for this in the future.