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

Allow ESM module for custom middlewares #14280

Merged
merged 10 commits into from
Oct 27, 2022
Merged

Allow ESM module for custom middlewares #14280

merged 10 commits into from
Oct 27, 2022

Conversation

5ika
Copy link
Contributor

@5ika 5ika commented Aug 31, 2022

What does it do?

Fix issue #14146

Why is it needed?

With TS version of Strapi, we can use ESM module imports but we can't import custom middleware in .ts module file.

How to test it?

See #14146

Related issue(s)/PR(s)

#14146

@strapi-cla
Copy link

strapi-cla commented Aug 31, 2022

CLA assistant check
All committers have signed the CLA.

@5ika 5ika changed the title Allow ESM module for custom middlewares #14146 Allow ESM module for custom middlewares Aug 31, 2022
@remidej remidej requested a review from Convly September 5, 2022 09:46
@remidej remidej added pr: fix This PR is fixing a bug source: typescript Source is related to TypeScript (typings, tooling, ...) labels Sep 5, 2022
Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

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

Hey, thank you for taking the time to open a PR,

If we want to enable loading ESM here, we could use the import-default util from here and move it to the @strapi/utils package, so that we're consistent with the rest of the logic where we allow using ESMs.

@5ika 5ika marked this pull request as draft September 5, 2022 10:37
@5ika
Copy link
Contributor Author

5ika commented Sep 5, 2022

@Convly Thanks ! I've updated the code to use import-default function.
I'm not sure I understand the last part of your comment: you mean move the function to @strapi/utils then update all occurrences in the code base ?

@5ika 5ika marked this pull request as ready for review September 7, 2022 08:53
@Convly
Copy link
Member

Convly commented Oct 17, 2022

@Convly Thanks ! I've updated the code to use import-default function. I'm not sure I understand the last part of your comment: you mean move the function to @strapi/utils then update all occurrences in the code base ?

Hey, really sorry for the late answer, it got lost in the Github notifications.
That's what I was suggesting indeed, we could move the content of this file to the strapi/utils package.

@5ika
Copy link
Contributor Author

5ika commented Oct 17, 2022

Hey, really sorry for the late answer, it got lost in the Github notifications. That's what I was suggesting indeed, we could move the content of this file to the strapi/utils package.

@Convly No problem for the delay and thank you for your response. I've moved import-default to strapi/utils package as asked 👍

Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

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

Thanks, I'm afraid those files would break if we don't change the importDefault require

  • /packages/core/strapi/lib/core/loaders/apis.js
  • /packages/core/strapi/lib/core/loaders/middlewares.js
  • /packages/core/strapi/lib/core/loaders/policies.js
  • /packages/core/strapi/lib/core/loaders/src-index.js

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Base: 58.76% // Head: 50.21% // Decreases project coverage by -8.54% ⚠️

Coverage data is based on head (7bebefb) compared to base (8a72fcd).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14280      +/-   ##
==========================================
- Coverage   58.76%   50.21%   -8.55%     
==========================================
  Files        1322      280    -1042     
  Lines       32043     9698   -22345     
  Branches     5988     2121    -3867     
==========================================
- Hits        18829     4870   -13959     
+ Misses      11352     3979    -7373     
+ Partials     1862      849    -1013     
Flag Coverage Δ
front ?
unit 50.21% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
packages/core/utils/lib/import-default.js 60.00% <ø> (ø)
packages/core/utils/lib/index.js 100.00% <100.00%> (ø)
packages/core/email/admin/src/utils/schema.js
...Page/components/LogoModalStepper/ImageCardAsset.js
...n/admin/src/components/LanguageProvider/reducer.js
.../admin/src/components/FormModal/component/index.js
...es/AuthPage/components/FieldActionWrapper/index.js
...ts/EditViewDataManagerProvider/utils/moveFields.js
...onents/GuidedTour/utils/arePreviousSectionsDone.js
...ore/admin/admin/src/pages/ProfilePage/utils/api.js
... and 1036 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.

@5ika
Copy link
Contributor Author

5ika commented Oct 21, 2022

Thanks, I'm afraid those files would break if we don't change the importDefault require

* /packages/core/strapi/lib/core/loaders/apis.js

* /packages/core/strapi/lib/core/loaders/middlewares.js

* /packages/core/strapi/lib/core/loaders/policies.js

* /packages/core/strapi/lib/core/loaders/src-index.js

Oh, sorry. I thought I've updated all occurrences. It's now updated.
Thank you

@Convly Convly self-requested a review October 24, 2022 07:29
Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for your contribution 🚀

@Convly
Copy link
Member

Convly commented Oct 25, 2022

(I'll just do a last round of verification (probably today or tomorrow) before merging)

@Convly Convly merged commit 364629a into strapi:main Oct 27, 2022
@Convly Convly added this to the 4.4.6 milestone Oct 27, 2022
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants