Skip to content

Find database migrations dir in typescript project - #18010

Closed
DevDengChao wants to merge 24 commits into
strapi:mainfrom
DevDengChao:feat/find-database-migrations-dir-in-ts-project
Closed

DevDengChao wants to merge 24 commits into
strapi:mainfrom
DevDengChao:feat/find-database-migrations-dir-in-ts-project

Conversation

@DevDengChao

Copy link
Copy Markdown
Contributor

What does it do?

Recongnize /dist/database/migrations in ts project

Why is it needed?

#17820

How to test it?

  • There are unit tests.
  • Or you can create an empty strapi project and write some typescript migrations.

Related issue(s)/PR(s)

#17820

Signed-off-by: DevDengChao <2325690622@qq.com>
Signed-off-by: DevDengChao <2325690622@qq.com>
@strapi-cla

strapi-cla commented Sep 11, 2023 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@DevDengChao DevDengChao changed the title Find database migrations dir in ts project Find database migrations dir in typescript project Sep 11, 2023
@DevDengChao

This comment was marked as spam.

@DevDengChao

This comment was marked as spam.

@DevDengChao

Copy link
Copy Markdown
Contributor Author

Related to #17960

@derrickmehaffy

Copy link
Copy Markdown
Member

I'll mention it to the team on Monday

@innerdvations innerdvations added source: typescript Source is related to TypeScript (typings, tooling, ...) pr: fix This PR is fixing a bug labels Sep 18, 2023
@innerdvations innerdvations added this to the 4.14.0 milestone Sep 18, 2023

@innerdvations innerdvations left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By the way, I currently have another PR where I fix an issue with the dialect initialization.

I'll have to think about how to reconcile how that works with this one but it shouldn't be very complex.

Comment thread packages/core/database/lib/index.js Outdated
@DevDengChao

This comment was marked as spam.

@DevDengChao

This comment was marked as spam.

Signed-off-by: DevDengChao <2325690622@qq.com>
Comment on lines 52 to 57
static async init(config: DatabaseConfig) {
const db = new Database(config);
await db.setup();
await validateDatabase(db);
return db;
}

@DevDengChao DevDengChao Oct 17, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@innerdvations If we can not use an async constructor, then we need to setup the db asynchronously, which causes the entityManager to be decleared nullable, while the entityManager is used widely in the project, and brings more things need to be updated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The root cause is that is-using-typescript checks tsconfig.json asynchronously:

module.exports = (dir, filename = undefined) => {
  const filePath = getConfigPath(dir, { filename });

  return fse.pathExists(filePath); // Promise<boolean>
};

Any idea about replace pathExists with pathExistsSync ?

@innerdvations innerdvations Oct 17, 2023 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Making path synchronous is a workaround I'd rather avoid. If we can make it work "properly" with everything relying on an async action happening in/after the setup, that would be best. We can keep everything that works synchronously in the constructor, and then move everything after that to the async setup method and await it in init (basically what you've done already)

I will try to give this PR some closer attention this week :)

Signed-off-by: DevDengChao <2325690622@qq.com>
@@ -1,5 +1,5 @@
// eslint-disable-next-line no-var
declare var strapi: any;
declare var strapi: any & { dirs: { app: { root } } };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be defined in the strapi typings

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

May I ask for help?

The @strapi/database package dose not depend on @strapi/types, so I cannot import { Strapi } from '@strapi/types/src';.
Even I can add @strapi/types to @strapi/database's devDependencies, it causes a circular dependencies.
So what should I do ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or maybe someone could update the strapi defined in global.d.ts first so I could follow it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't pay enough attention before to realize what was happening. If this is already defined as 'any' why do we need to add anything to it? Shouldn't it work without changing anything here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It works without this change, I added it for better DX when writing unit tests..
Then should we keep it or remove it ?

Comment thread packages/core/database/src/__tests__/index.test.ts Outdated
@innerdvations

Copy link
Copy Markdown
Contributor

Changing those db properties to optional will be a breaking change for typescript, so we'll have to do it in a bit of a hacky way for Strapi v4 and then do it better in v5.

I am working on a new PR to address that specifically and will credit you for your work on the issue, and then once that's merged, the TS migrations issue can be done in a much simpler PR.

innerdvations added a commit that referenced this pull request Oct 20, 2023
Based on #18010 by @DevDengChao
Co-authored-by: DevDengChao <2325690622@qq.com>
@DevDengChao

Copy link
Copy Markdown
Contributor Author

@innerdvations Good to hear that. Subscribing the other PR now.

@Marc-Roig Marc-Roig modified the milestones: 4.15.0, 4.15.1 Oct 23, 2023
@Marc-Roig Marc-Roig modified the milestones: 4.15.1, 4.15.2 Oct 30, 2023
@alexandrebodin alexandrebodin modified the milestones: 4.15.2, 4.15.3 Nov 3, 2023
@alexandrebodin alexandrebodin removed this from the 4.15.3 milestone Nov 11, 2023
@alexandrebodin

Copy link
Copy Markdown
Member

Important

With the release of Strapi 5, we have lifted the code freeze. To ensure we can effectively review and assist with contributions, we are now only accepting PRs targeting version 5 (the develop branch).

As your PR targets version 4, it has been automatically closed. However, we would greatly appreciate it if you could update your contribution to align with version 5 and resubmit!

@DevDengChao
DevDengChao deleted the feat/find-database-migrations-dir-in-ts-project branch September 19, 2024 08:39
@DevDengChao

Copy link
Copy Markdown
Contributor Author

@alexandrebodin Hi, I created another PR here #21374 , do you have time review it ?

@alexandrebodin

alexandrebodin commented Sep 19, 2024 •

Copy link
Copy Markdown
Member

This was fast thank you ! I'll assign someone We are aiming to review everything in the next month 🙏

This branch was successfully deployed

1 active deployment
Preview — fdae912d Deployed Oct 19, 2023 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: fix This PR is fixing a bug source: typescript Source is related to TypeScript (typings, tooling, ...)

Projects

Status: Fixed/Shipped

Development

Successfully merging this pull request may close these issues.

8 participants