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 Patternfly example to not use deprecated techniques #20916

Closed
willieseabrook opened this issue Jan 8, 2021 · 9 comments
Closed

Fix Patternfly example to not use deprecated techniques #20916

willieseabrook opened this issue Jan 8, 2021 · 9 comments
Assignees
Labels
examples Issue/PR related to examples good first issue Easy to fix issues, good for newcomers please verify canary The issue should be verified against next@canary. It will be closed after 30 days of inactivity

Comments

@willieseabrook
Copy link

willieseabrook commented Jan 8, 2021

What example does this report relate to?

with-patternfly

What version of Next.js are you using?

10.0.4

What version of Node.js are you using?

14.15.4

What browser are you using?

Chrome (all, not browser related)

What operating system are you using?

Linux

How are you deploying your application?

yarn run dev

Describe the Bug

Patternfly is becoming very, very popular, and there's quite a few bug/issue reports scattered around of people fighting to get it to work with Nextjs

Unfortunately, the Patternfly example https://github.com/vercel/next.js/tree/canary/examples/with-patternfly uses deprecated techniques:

  • @zeit/next-css which is deprecated and removed from repository. Running this example now gives a warning and will probably soon fail as nextjs development continues
  • It uses an old next-transpile-modules version 4.1.0 where latest is 6.0.0
  • And while the current example doesn't use it, the only way to get SASS working is with the also deprecated and removed @zeit/next-sass module, which in turn will only work with the deprecated node-sass module when used with Patternfly (See my issue filed to dart-sass at Nextjs + Patternfly react table head + using legacy node-sass works, but using dart-sass fails sass/dart-sass#1186 )

Expected Behavior

Update the Patternfly example to use supported techniques and/or an updated workaround.

I have a repo at https://github.com/willieseabrook/nextjs-with-patternfly which at least uses the latest version of https://github.com/martpie/next-transpile-modules

Note, that solving this for Patternfly will be very valuable for the wider community, as the reason the with-patternfly example is complicated is because Patternfly includes css files in its JS files in the node_modules directory.

In various issues I've found, the Nextjs team has expressly prohibited doing this as a (very very good) design decision, so that will not change.

However, it would be nice to have a standard workaround, using Patternfly as an example, for people who have no other choice - I can't control Patternfly and this breaks quite a few other React modules (hence the various scattered issues on this topic)

Without a new standard workaround, Patternfly would need to be considered expressly incompatible with Nextjs and therefore need to be removed from the examples repository.

To Reproduce

Try to run the example at https://github.com/vercel/next.js/tree/canary/examples/with-patternfly

@willieseabrook willieseabrook added bug Issue was opened via the bug report template. examples Issue/PR related to examples labels Jan 8, 2021
@willieseabrook
Copy link
Author

Also as on approach for an accepted workaround which would be quite simple and self-contained in next.config.js rather than using next-transpile-modules (which does do similar things), see here https://github.com/RedHatInsights/frontend-components/blob/docs/packages/docs/next.config.js#L13

It's from a Patternfly discussion patternfly/patternfly-react#4125 by @Hyperkid123 who appears to be using Nextjs + Patternfly for a Redhat project.

@willieseabrook
Copy link
Author

And also pinging @martpie who as the maintainer of next-transpile-modules appears to be an expert in this topic area (and btw Pierre thank you so much for your module!!!)

@martpie
Copy link
Contributor

martpie commented Jan 8, 2021

How can I help? 😇

@willieseabrook
Copy link
Author

@martpie I think your module next-transpile-modules is stable and well maintained.

So the part that needs to be worked on is to allow someone to use nextjs + patternfly + css + sass without needing the now deprecated modules next-css and next-sass - e.g using the built in css and sass support.

I spent 3 days in my next.config.js trying to modify and remove the rules the nextjs team have inserted to block css in node_modules and disable css support, but I just couldn't get it to work - I'm not proficient at all in nextjs and an a complete webpack and babel beginner.

I was able to find the rules and change them, but it just didn't seem to work

@timneutkens timneutkens added good first issue Easy to fix issues, good for newcomers and removed bug Issue was opened via the bug report template. labels Jan 11, 2021
@ghost
Copy link

ghost commented Mar 31, 2021

@timneutkens, @martpie, any updates on this issue? We still have to use the approach mentioned in patternfly/patternfly-react#4125 discussion for Next.js to work with Patternfly.

@ghost
Copy link

ghost commented Apr 21, 2021

@willieseabrook, there are some issues with using custom next.config.js to make Next.js work with Patternfly.
Here's my next.config.js:

const path = require('path');
const withCss = require('@zeit/next-css');

const BG_IMAGES_DIRNAME = 'bgimages';

const withTM = require('next-transpile-modules')(
  [
    '@patternfly/react-core',
    '@patternfly/react-icons',
    '@patternfly/react-styles',
    '@patternfly/react-table',
    '@patternfly/react-tokens',
  ],
  { debug: false }
);

module.exports = withCss(
  withTM({
    webpack: (config, { buildId, dev, isServer, defaultLoaders, webpack }) => {
      config.module.rules.push({
        test: /\.(svg|ttf|eot|woff|woff2)$/,
        include: [
          path.resolve(__dirname, 'node_modules/patternfly/dist/fonts'),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/react-core/dist/styles/assets/fonts'
          ),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/react-core/dist/styles/assets/pficon'
          ),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/patternfly/assets/fonts'
          ),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/patternfly/assets/pficon'
          ),
          path.resolve(__dirname, 'node_modules/react-multi-carousel/lib'),
        ],
        use: {
          loader: 'file-loader',
          options: {
            limit: 5000,
            publicPath: '/_next/static/fonts/',
            outputPath: 'static/fonts/',
            name: '[name].[ext]',
            esModule: false,
          },
        },
      });

      config.module.rules.push({
        test: /\.svg$/,
        include: (input) => input.indexOf('background-filter.svg') > 1,
        use: [
          {
            loader: 'url-loader',
            options: {
              limit: 5000,
              publicPath: '/_next/static/svgs/',
              outputPath: 'static/svgs/',
              name: '[name].[ext]',
            },
          },
        ],
      });

      config.module.rules.push({
        test: /\.svg$/,
        include: (input) => input.indexOf(BG_IMAGES_DIRNAME) > -1,
        use: {
          loader: 'svg-url-loader',
          options: {},
        },
      });

      config.module.rules.push({
        test: /\.svg$/,
        include: (input) =>
          input.indexOf(BG_IMAGES_DIRNAME) === -1 &&
          input.indexOf('fonts') === -1 &&
          input.indexOf('background-filter') === -1 &&
          input.indexOf('pficon') === -1,
        use: {
          loader: 'raw-loader',
          options: {},
        },
      });

      config.module.rules.push({
        test: /\.(jpg|jpeg|png|gif)$/i,
        include: [
          path.resolve(__dirname, 'src'),
          path.resolve(__dirname, 'node_modules/patternfly'),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/patternfly/assets/images'
          ),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/react-styles/css/assets/images'
          ),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/react-core/dist/styles/assets/images'
          ),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/react-core/node_modules/@patternfly/react-styles/css/assets/images'
          ),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/react-table/node_modules/@patternfly/react-styles/css/assets/images'
          ),
          path.resolve(
            __dirname,
            'node_modules/@patternfly/react-inline-edit-extension/node_modules/@patternfly/react-styles/css/assets/images'
          ),
        ],
        use: [
          {
            loader: 'url-loader',
            options: {
              limit: 5000,
              publicPath: '/_next/static/images/',
              outputPath: 'static/images/',
              name: '[name].[ext]',
            },
          },
        ],
      });

      return config;
    },
  })
);

When using it we also needed to install Webpack 4 as dev dependency.
But the performance in development is very poor. In this case, as I understand it, the node process starts rebuilding the whole patternfly library with our project on every render/rerender which causes high CPU and RAM usage:
image

This time around 40-60% CPU load and 1-1.5 GB of RAM are required. The first compilation will take about 4 minutes.

A couple of times the node process even crashed with GC report. I believe that this behavior not suits for development.

Also there were no such problems when running the project in production after build. It won't cause any performance issues in production mode but the high CPU and RAM usage and the time it takes to compile everything really pisses me off.

fabianishere added a commit to fabianishere/next.js that referenced this issue May 22, 2021
This change fixes the issues with the PatternFly example showing how to
use PatternFly 4 in conjunction with Next.js:

1. next-transpile-modules has been updated to 7.2.0, which adds support
   for Global CSS imports used by PatternFly 4. This eliminates the
   custom Webpack modification that were necessary previously.
2. All dependencies have been updated to the latest version.
3. Documentation has been updated to include troubleshooting steps.

Addresses vercel#20916
kodiakhq bot pushed a commit that referenced this issue May 22, 2021
This change fixes the issues with the PatternFly example showing how to
use PatternFly 4 in conjunction with Next.js:

1. next-transpile-modules has been updated to 7.2.0, which adds support
   for Global CSS imports used by PatternFly 4. This eliminates the
   custom Webpack modification that were necessary previously.
2. All dependencies have been updated to the latest version.
3. Documentation has been updated to include troubleshooting steps.

Addresses #20916
flybayer pushed a commit to blitz-js/next.js that referenced this issue Jun 1, 2021
This change fixes the issues with the PatternFly example showing how to
use PatternFly 4 in conjunction with Next.js:

1. next-transpile-modules has been updated to 7.2.0, which adds support
   for Global CSS imports used by PatternFly 4. This eliminates the
   custom Webpack modification that were necessary previously.
2. All dependencies have been updated to the latest version.
3. Documentation has been updated to include troubleshooting steps.

Addresses vercel#20916
@jankaifer jankaifer self-assigned this Dec 1, 2022
@jankaifer jankaifer added the please verify canary The issue should be verified against next@canary. It will be closed after 30 days of inactivity label Dec 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

Please verify that your issue can be recreated with next@canary.

Why was this issue marked with the please verify canary label?

We noticed the provided reproduction was using an older version of Next.js, instead of canary.

The canary version of Next.js ships daily and includes all features and fixes that have not been released to the stable version yet. You can think of canary as a public beta. Some issues may already be fixed in the canary version, so please verify that your issue reproduces by running npm install next@canary and test it in your project, using your reproduction steps.

If the issue does not reproduce with the canary version, then it has already been fixed and this issue can be closed.

How can I quickly verify if my issue has been fixed in canary?

The safest way is to install next@canary in your project and test it, but you can also search through closed Next.js issues for duplicates or check the Next.js releases.

My issue has been open for a long time, why do I need to verify canary now?

Next.js does not backport bug fixes to older versions of Next.js. Instead, we are trying to introduce only a minimal amount of breaking changes between major releases.

What happens if I don't verify against the canary version of Next.js?

An issue with the please verify canary that receives no meaningful activity (e.g. new comments that acknowledge verification against canary) will be automatically closed and locked after 30 days.

If your issue has not been resolved in that time and it has been closed/locked, please open a new issue, with the required reproduction, using next@canary.

I did not open this issue, but it is relevant to me, what can I do to help?

Anyone experiencing the same issue is welcome to provide a minimal reproduction following the above steps. Furthermore, you can upvote the issue using the 👍 reaction on the topmost comment (please do not comment "I have the same issue" without repro steps). Then, we can sort issues by votes to prioritize.

I think my reproduction is good enough, why aren't you looking into it quicker?

We look into every Next.js issue and constantly monitor open issues for new comments.

However, sometimes we might miss one or two due to the popularity/high traffic of the repository. We apologize, and kindly ask you to refrain from tagging core maintainers, as that will usually not result in increased priority.

Upvoting issues to show your interest will help us prioritize and address them as quickly as possible. That said, every issue is important to us, and if an issue gets closed by accident, we encourage you to open a new one linking to the old issue and we will look into it.

Useful Resources

@balazsorban44
Copy link
Member

This issue has been automatically closed because it wasn't verified against next@canary. If you think it was closed by accident, please leave a comment. If you are running into a similar issue, please open a new issue with a reproduction. Thank you.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
examples Issue/PR related to examples good first issue Easy to fix issues, good for newcomers please verify canary The issue should be verified against next@canary. It will be closed after 30 days of inactivity
Projects
None yet
Development

No branches or pull requests

5 participants