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): bring back previous SVG and SVGR behavior for React projects #22628

Merged
merged 1 commit into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
80 changes: 80 additions & 0 deletions e2e/next-extensions/src/next-svgr.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import {
cleanupProject,
createFile,
listFiles,
newProject,
readFile,
runCLI,
runE2ETests,
uniq,
updateFile,
} from '@nx/e2e/utils';

describe('NextJs SVGR support', () => {
beforeAll(() => {
newProject({
packages: ['@nx/next'],
});
});

afterAll(() => cleanupProject());

it('should allow both SVG asset and SVGR component to be used', () => {
const appName = uniq('app');
runCLI(
`generate @nx/next:app ${appName} --no-interactive --appDir=true --src=true`
);
createFile(
`apps/${appName}/src/app/nx.svg`,
`
<svg version="1.1" width="300" height="200" xmlns="http://www.w3.org/2000/svg">
<text x="150" y="125" font-size="60" text-anchor="middle" fill="white">SVG for app</text>
</svg>
`
);
updateFile(
`apps/${appName}/src/app/page.tsx`,
`
import Image from 'next/image';
import svgImg, { ReactComponent as Logo } from './nx.svg';
export default async function Index() {
return (
<>
<Image src={svgImg} alt="Alt for SVG img tag" />
<Logo />
</>
);
}
`
);
updateFile(
`apps/${appName}/next.config.js`,
`
const { composePlugins, withNx } = require('@nx/next');
const nextConfig = {
nx: {
svgr: true,
},
};
const plugins = [
withNx,
];
module.exports = composePlugins(...plugins)(nextConfig);
`
);

runCLI(`build ${appName}`);

const pageFile = readFile(`apps/${appName}/.next/server/app/page.js`);
const svgFile = listFiles(`apps/${appName}/.next/static/media`).find((f) =>
/nx\.[a-z0-9]+\.svg$/.test(f)
);
expect(`apps/${appName}/.next/static/chunks/app/${pageFile}`).toMatch(
/SVG for app/
);
expect(`apps/${appName}/.next/static/chunks/app/${pageFile}`).toMatch(
/Alt for SVG img tag/
);
expect(svgFile).toBeTruthy();
});
});
65 changes: 65 additions & 0 deletions e2e/react-extensions/src/react-webpack.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import {
cleanupProject,
createFile,
listFiles,
newProject,
readFile,
runCLI,
runCLIAsync,
uniq,
updateFile,
} from '@nx/e2e/utils';

describe('Build React applications and libraries with Vite', () => {
beforeAll(() => {
newProject({
packages: ['@nx/react'],
});
});

afterAll(() => {
cleanupProject();
});

// Regression test: https://github.com/nrwl/nx/issues/21773
it('should support SVGR and SVG asset in the same project', async () => {
const appName = uniq('app');

runCLI(
`generate @nx/react:app ${appName} --bundler=webpack --compiler=babel --unitTestRunner=none --no-interactive`
);
createFile(
`apps/${appName}/src/app/nx.svg`,
`
<svg version="1.1" width="300" height="200" xmlns="http://www.w3.org/2000/svg">
<text x="150" y="125" font-size="60" text-anchor="middle" fill="white">SVG for app</text>
</svg>
`
);
updateFile(
`apps/${appName}/src/app/app.tsx`,
`
import svgImg, { ReactComponent as Logo } from './nx.svg';
export function App() {
return (
<>
<img src={svgImg} alt="Alt for SVG img tag" />
<Logo />
</>
);
}
export default App;
`
);

await runCLIAsync(`build ${appName}`);

const outFiles = listFiles(`dist/apps/${appName}`);
const mainFile = outFiles.find((f) => f.startsWith('main.'));
const mainContent = readFile(`dist/apps/${appName}/${mainFile}`);
const svgFile = outFiles.find((f) => f.endsWith('.svg'));
expect(mainContent).toMatch(/SVG for app/);
expect(mainContent).toMatch(/Alt for SVG img tag/);
expect(svgFile).toBeTruthy();
}, 300_000);
});
1 change: 1 addition & 0 deletions packages/next/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
"webpack",
// require.resovle is used for these
"@babel/plugin-proposal-decorators",
"file-loader",
"url-loader",
"@svgr/webpack"
]
Expand Down
1 change: 1 addition & 0 deletions packages/next/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"@svgr/webpack": "^8.0.1",
"chalk": "^4.1.0",
"copy-webpack-plugin": "^10.2.4",
"file-loader": "^6.2.0",
"fs-extra": "^11.1.0",
"ignore": "^5.0.4",
"semver": "^7.5.3",
Expand Down
67 changes: 1 addition & 66 deletions packages/next/plugins/with-nx.spec.ts
Original file line number Diff line number Diff line change
@@ -1,69 +1,4 @@
import { NextConfigComplete } from 'next/dist/server/config-shared';
import { getAliasForProject, getNextConfig } from './with-nx';

describe('getNextConfig', () => {
describe('svgr', () => {
it('should be used by default', () => {
const config = getNextConfig();

const result = config.webpack(
{
module: { rules: [{ oneOf: [] }] },
},
{
buildId: 'build-id',
config: config as NextConfigComplete,
dev: true,
dir: 'dir',
isServer: false,
totalPages: 0,
webpack: undefined,
defaultLoaders: {
babel: {
options: {},
},
},
}
);

expect(
result.module.rules.some((rule) => rule.test?.test('cat.svg'))
).toBe(true);
});

it('should not be used when disabled', () => {
const config = getNextConfig({
nx: {
svgr: false,
},
});

const result = config.webpack(
{
module: { rules: [{ oneOf: [] }] },
},
{
buildId: 'build-id',
config: config as NextConfigComplete,
dev: true,
dir: 'dir',
isServer: false,
totalPages: 0,
webpack: undefined,
defaultLoaders: {
babel: {
options: {},
},
},
}
);

expect(
result.module.rules.some((rule) => rule.test?.test('cat.svg'))
).toBe(false);
});
});
});
import { getAliasForProject } from './with-nx';

describe('getAliasForProject', () => {
it('should return the matching alias for a project', () => {
Expand Down
63 changes: 48 additions & 15 deletions packages/next/plugins/with-nx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,22 +337,55 @@ export function getNextConfig(

// Default SVGR support to be on for projects.
if (nx?.svgr !== false) {
config.module.rules.push(
// Apply rule for svg imports ending in ?url
{
test: /\.svg$/i,
type: 'asset',
resourceQuery: /url/, // apply to *.svg?url
// TODO(v20): Remove file-loader and use `?react` querystring to differentiate between asset and SVGR.
// It should be:
// use: [{
// test: /\.svg$/i,
// type: 'asset',
// resourceQuery: /react/, // *.svg?react
// },
// {
// test: /\.svg$/i,
// issuer: /\.[jt]sx?$/,
// resourceQuery: { not: [/react/] }, // exclude react component if *.svg?react
// use: ['@svgr/webpack'],
// }],
// See:
// - SVGR: https://react-svgr.com/docs/webpack/#use-svgr-and-asset-svg-in-the-same-project
// - Vite: https://www.npmjs.com/package/vite-plugin-svgr
// - Rsbuild: https://github.com/web-infra-dev/rsbuild/pull/1783
// Note: We also need a migration for any projects that are using SVGR to convert
// `import { ReactComponent as X } from './x.svg` to
// `import X from './x.svg?react';
config.module.rules.push({
test: /\.svg$/,
issuer: { not: /\.(css|scss|sass)$/ },
resourceQuery: {
not: [
/__next_metadata__/,
/__next_metadata_route__/,
/__next_metadata_image_meta__/,
],
},

// Convert all other svg imports to React components
{
test: /\.svg$/i,
issuer: /\.[jt]sx?$/,
resourceQuery: { not: [/url/] },
use: ['@svgr/webpack'],
}
);
use: [
{
loader: require.resolve('@svgr/webpack'),
options: {
svgo: false,
titleProp: true,
ref: true,
},
},
{
loader: require.resolve('file-loader'),
options: {
// Next.js hard-codes assets to load from "static/media".
// See: https://github.com/vercel/next.js/blob/53d017d/packages/next/src/build/webpack-config.ts#L1993
name: 'static/media/[name].[hash].[ext]',
},
},
],
});
}

return userWebpack(config, options);
Expand Down
1 change: 1 addition & 0 deletions packages/react/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
"babel-plugin-emotion",
"babel-plugin-styled-components",
"css-loader",
"file-loader",
"less-loader",
"react-refresh",
"rollup",
Expand Down
1 change: 1 addition & 0 deletions packages/react/package.json
jaysoo marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"@phenomnomnominal/tsquery": "~5.0.1",
"@svgr/webpack": "^8.0.1",
"chalk": "^4.1.0",
"file-loader": "^6.2.0",
"minimatch": "9.0.3",
"tslib": "^2.3.0",
"@nx/devkit": "file:../devkit",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,26 @@ export function applyReactConfig(
if (options.svgr !== false) {
removeSvgLoaderIfPresent(config);

// TODO(v20): Remove file-loader and use `?react` querystring to differentiate between asset and SVGR.
// It should be:
// use: [{
// test: /\.svg$/i,
// type: 'asset',
// resourceQuery: /react/, // *.svg?react
// },
// {
// test: /\.svg$/i,
// issuer: /\.[jt]sx?$/,
// resourceQuery: { not: [/react/] }, // exclude react component if *.svg?react
// use: ['@svgr/webpack'],
// }],
// See:
// - SVGR: https://react-svgr.com/docs/webpack/#use-svgr-and-asset-svg-in-the-same-project
// - Vite: https://www.npmjs.com/package/vite-plugin-svgr
// - Rsbuild: https://github.com/web-infra-dev/rsbuild/pull/1783
// Note: We also need a migration for any projects that are using SVGR to convert
// `import { ReactComponent as X } from './x.svg` to
// `import X from './x.svg?react';
config.module.rules.push({
test: /\.svg$/,
issuer: /\.(js|ts|md)x?$/,
Expand All @@ -23,6 +43,12 @@ export function applyReactConfig(
ref: true,
},
},
{
loader: require.resolve('file-loader'),
options: {
name: '[name].[hash].[ext]',
},
},
],
});
}
Expand Down