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

Fixed getMigration filetype for built versions #366

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

d-tsaruk
Copy link
Contributor

Pull Request Description:

Subject: [PATCH] Fixed getMigrations filetype for built versions

Summary:

This patch addresses an issue with the getMigrations function in src/migrator.ts where it was only filtering .ts files for migrations. The fix now allows the function to correctly handle both .ts and .js files, ensuring compatibility with built versions of the application that use JavaScript files instead of TypeScript.

Changes:

  • Updated the getMigrations method to filter both .ts and .js files in the migrations folder.
  • Introduced a conditional check to support both development and production environments, ensuring that migrations can be found whether the application is running with TypeScript or after being built to JavaScript.

Impact:

This change ensures that migrations can be correctly loaded regardless of whether the application is running in development mode (using TypeScript) or in production (using JavaScript after the build).

Related Issues:

  • Fixes migration file loading issue for production builds.

Testing:

  • Ensure that both .ts and .js migration files are correctly detected and processed when the application is built and deployed.

Additional Notes:

  • The modification simplifies handling of migration files across environments and ensures smoother operation during the build and deployment phases.

@ilovepixelart
Copy link
Owner

ilovepixelart commented Dec 10, 2024

Hello @d-tsaruk thanks for the contribution, I see the issue you trying to solve, can I ask you to include some test in scope of this change.

Also in case you are not aware, you don't actually need to compile migration files, you can simply put them outside of your src folder and exclude them from tsconfig.json like shown here: https://github.com/ilovepixelart/ts-express-swc this is the intended way to use this package

To test what described above you can clone mentioned project and just run commands bellow

npm i
npm run build:ts
npm run start

@d-tsaruk
Copy link
Contributor Author

Hello @d-tsaruk thanks for the contribution, I see the issue you trying to solve, can I ask you to include some test in scope of this change.

Also in case you are not aware you don't actually need to compile migration files, you can simply put them, outside of your src folder like shown here: https://github.com/ilovepixelart/ts-express-swc this is the intended way to use this package

npm i
npm run build:ts
npm run start

Hi. I'll add tests

Yep I understand that i can move migration files from my main source folder to not build its. but it make some another problems.

In this case i can use mongoose models in migrations and it very comfortable for me in case my application. but if i'm not build migration files - i should not build (or copy) mongoose model files...

@ilovepixelart
Copy link
Owner

Regarding biome issue in approved run, you can simply run command bellow, it should handle the case automatically

npm run biome:fix

@ilovepixelart ilovepixelart added the bug Something isn't working label Dec 10, 2024
@d-tsaruk
Copy link
Contributor Author

@ilovepixelart biome fixed

@d-tsaruk
Copy link
Contributor Author

i think that we don't need add tests for .js extension, because its just for builded files

@ilovepixelart
Copy link
Owner

ilovepixelart commented Dec 12, 2024

Ok, we are not generating .js files anyway lets push

@ilovepixelart ilovepixelart merged commit 042ac8c into ilovepixelart:main Dec 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants