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

[webpack]: use path.resolve instead of require #14848

Merged
merged 5 commits into from Nov 17, 2022

Conversation

joshuaellis
Copy link
Member

What does it do?

Replaces usage of require.resolve with path.resolve in the admin webpack alias-ing config part.

Why is it needed?

Because require.resolve demands the CJS version of a library therefore not allowing the ESM to be used in bundling (which is typically better treeshaked)

How to test it?

I'd like this to be released as a beta on wednesday so we can discuss with some members of the community to solve their issues

Related issue(s)/PR(s)

because this assumes we must always use CJS
@joshuaellis joshuaellis added source: core:admin Source is core/admin package pr: chore This PR contains chore tasks (cleanups, configs, tooling...) labels Nov 14, 2022
@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

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

Coverage data is based on head (e05abf9) compared to base (8c20ea2).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14848   +/-   ##
=======================================
  Coverage   59.76%   59.76%           
=======================================
  Files        1339     1339           
  Lines       32669    32669           
  Branches     6189     6189           
=======================================
  Hits        19526    19526           
  Misses      11296    11296           
  Partials     1847     1847           
Flag Coverage Δ
front 64.28% <ø> (ø)
unit 49.80% <ø> (ø)

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.

@joshuaellis
Copy link
Member Author

@alexandrebodin I've opted to use find-root because we can pass it the string we were using before (the CJS module) and it will relatively find the package.json relative to that file therefore allowing the bundler to pick appropriately the correct file type.

More confident in this solution going forward as well because it's extremely close to our current implementation.

@alexandrebodin
Copy link
Member

@alexandrebodin I've opted to use find-root because we can pass it the string we were using before (the CJS module) and it will relatively find the package.json relative to that file therefore allowing the bundler to pick appropriately the correct file type.

More confident in this solution going forward as well because it's extremely close to our current implementation.

Interesting it looks like it's doing something like we do in the backend here packages/core/strapi/lib/load/package-path.js. Could we use that method ?

@joshuaellis
Copy link
Member Author

Interesting it looks like it's doing something like we do in the backend here packages/core/strapi/lib/load/package-path.js. Could we use that method ?

Thanks for letting me know, yes I think that function will work nicely for our needs!

@joshuaellis joshuaellis modified the milestone: 4.5.1 Nov 15, 2022
@joshuaellis
Copy link
Member Author

@Convly I believe this is ready for an experimental release. Do you want to leave this PR, add dont merge flag and release from this branch or do you want to merge it into a different branch? or something else? 😄

@Convly
Copy link
Member

Convly commented Nov 15, 2022

If you can update this branch with main and add a "do not merge" label, we can release an experimental from this branch 🙂

@joshuaellis joshuaellis added the flag: don't merge This PR should not be merged at the moment label Nov 15, 2022
@Convly
Copy link
Member

Convly commented Nov 15, 2022

@joshuaellis

A new experimental release has been deployed for chore/use-path-instead-of-require under the 0.0.0-b4eaa97842f53ed5611c878ecef9654b7bde1c11 version.

To create a new application, use the following command

npx create-strapi-app@0.0.0-b4eaa97842f53ed5611c878ecef9654b7bde1c11 my-app --quickstart

@joshuaellis joshuaellis removed the flag: don't merge This PR should not be merged at the moment label Nov 16, 2022
@joshuaellis
Copy link
Member Author

@gu-stav can you review this please? I think we can merge it now 👍🏼

@joshuaellis joshuaellis merged commit 49bf569 into main Nov 17, 2022
@joshuaellis joshuaellis deleted the chore/use-path-instead-of-require branch November 17, 2022 09:01
@gu-stav gu-stav added this to the 4.5.2 milestone Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: core:admin Source is core/admin package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read properties of undefined in Content-Type Builder and Uploads pages
4 participants