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

[fix][webpack]: Use a dynamic list of exclude to allow TSX plugins to be transpiled #14674

Merged
merged 3 commits into from
Oct 24, 2022

Conversation

joshuaellis
Copy link
Member

@joshuaellis joshuaellis commented Oct 19, 2022

What does it do?

  • Changes the exclude pattern for webpack to not exclude the TSX plugins installed in node_modules by creating a dynamic regex.

Why is it needed?

Currently when a plugin-creator makes a fresh plugin with Typescript we do not provide a build step for the admin package they may create, therefore when they publish their plugin to npm it contains tsx|ts files. When this is imported into a strapi app and the build is triggered it fails because it is compiled.

How to test it?

  • cd examples/getstarted
  • yarn add strapi-plugin-point-list
  • yarn build

How did I come to this conclusion

I compared the memory profiles of building with different webpack configurations on a clean strapi app vs the current build process (without the plugin so it would actually work).

The problem

This is our current rule for webpack:

        {
          test: /\.tsx?$/,
          loader: require.resolve('esbuild-loader'),
          include: [cacheDir, ...pluginsPath],
          exclude: /node_modules/,
          options: {
            loader: 'tsx',
            target: 'es2015',
          },
        }

Current Benchmark

CleanShot 2022-10-19 at 15 54 51@2x

CleanShot 2022-10-19 at 15 54 59@2x

Solution 1 – Remove Exclude

CleanShot 2022-10-19 at 15 24 43@2x

CleanShot 2022-10-19 at 15 25 02@2x

The webpack config:

        {
          test: /\.tsx?$/,
          loader: require.resolve('esbuild-loader'),
          include: [cacheDir, ...pluginsPath],
          options: {
            loader: 'tsx',
            target: 'es2015',
          },
        }

Solution 2 – Create a dynamic exclude list

CleanShot 2022-10-19 at 15 28 17@2x

CleanShot 2022-10-19 at 15 28 02@2x

note – i've inlined this for illustration purposes only

        {
          test: /\.tsx?$/,
          loader: require.resolve('esbuild-loader'),
          include: [cacheDir, ...pluginsPath],
          exclude: new RegExp(`node_modules/(?!(${pluginsPath.map((dir) => dir.split('node_modules/')[1]).join('|')}))`)
          options: {
            loader: 'tsx',
            target: 'es2015',
          },
        }

Solution 3 – Only pass cacheDir as include

This solution did not work and failed to build.

        {
          test: /\.tsx?$/,
          loader: require.resolve('esbuild-loader'),
          include: [cacheDir],
          exclude: new RegExp(`node_modules/(?!(${pluginsPath.map((dir) => dir.split('node_modules/')[1]).join('|')}))`)
          options: {
            loader: 'tsx',
            target: 'es2015',
          },
        }

Methodology

The general flow is listed below:

pip3 install memory-profiler
pip3 install matplotlib

npx create-strapi-app@4.4.4 strapi-app --quickstart
cd strapi-app
yarn add strapi-plugin-point-list

/opt/homebrew/bin/mprof run node_modules/.bin/strapi build
/opt/homebrew/bin/mprof plot

Environment

  • Node: v16.17.0
  • OS: MacOS Monterey, 12.5
  • CPU: Apple M1 Pro

I also manually deleted the .cache folder after every build, just incase.
Also, if you wanna explore the dat files, see the zip below.

dat_files.zip

Related issue(s)/PR(s)

@joshuaellis joshuaellis added source: tooling Source is GitHub tooling/tests/ect pr: fix This PR is fixing a bug labels Oct 19, 2022
@joshuaellis
Copy link
Member Author

Special thanks to @gu-stav for teaching me about profiling 💅🏼

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Base: 58.76% // Head: 58.78% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (bd1cf01) compared to base (6f72541).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14674      +/-   ##
==========================================
+ Coverage   58.76%   58.78%   +0.01%     
==========================================
  Files        1322     1323       +1     
  Lines       32045    32059      +14     
  Branches     5989     5992       +3     
==========================================
+ Hits        18831    18845      +14     
  Misses      11351    11351              
  Partials     1863     1863              
Flag Coverage Δ
front 62.47% <ø> (ø)
unit 50.27% <100.00%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
...es/core/admin/utils/create-plugins-exclude-path.js 100.00% <100.00%> (ø)

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

@gu-stav, tested on windows (personally) and added some tests as well.

Copy link
Contributor

@gu-stav gu-stav left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for adding these great tests! 🌻

@joshuaellis joshuaellis merged commit a950015 into main Oct 24, 2022
@joshuaellis joshuaellis deleted the chore/webpack/fix-plugin-tsx branch October 24, 2022 10:21
@joshuaellis joshuaellis added this to the 4.4.6 milestone Oct 24, 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: tooling Source is GitHub tooling/tests/ect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom-field plugin written in typescript fails to load when loading from node_modules
3 participants