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 style compilation for file paths containing webpack template strings #1247

Merged
merged 14 commits into from
Feb 1, 2024

Conversation

askoufis
Copy link
Contributor

@askoufis askoufis commented Dec 2, 2023

Fixes #1069.

You'd think this would be easily google-able, but apparently not. Eventually found out how to escape these template strings in the webpack docs, but only after digging through the source code of TemplatedPathPlugin and working backwards from there.

This feels like a rather naive fix to me. I'm not sure if changing this output filename could have any unintended side-effects. Also, I wanted to add a test case to one of the fixtures but wasn't sure which one. Would low-level be the best place for it?
Using a more robust regex now, and added some unit tests and a webpack-specific fixture.

Copy link

changeset-bot bot commented Dec 2, 2023

🦋 Changeset detected

Latest commit: 0e17aac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@vanilla-extract/webpack-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

package.json Outdated Show resolved Hide resolved
@@ -2,7 +2,7 @@
"compilerOptions": {
"target": "ESNEXT" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */,
"module": "commonjs" /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */,
"lib": ["es2019", "es2017", "dom"],
"lib": ["es2021", "dom"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required for tsc to recognize replaceAll. It was added in node 15, so IMO it's safe to use.

@arturmuller
Copy link

This would be a very welcome fix!

packages/webpack-plugin/src/compiler.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mrm007 mrm007 self-requested a review January 29, 2024 13:00
@askoufis
Copy link
Contributor Author

Not quite sure where to put tests. Just in a random fixture, or should we make a webpack/nextjs specific one? #1203 sort of already paved the way for this a bit, although it's next-specific.

@askoufis
Copy link
Contributor Author

I've added unit tests and a fixture for the escaping functionality.

const templateStringRegexp = /\[([^\[\]\.]+)\]/g;

export const escapeWebpackTemplateString = (s: string) =>
s.replaceAll(templateStringRegexp, '[\\$1\\]');
Copy link
Contributor Author

@askoufis askoufis Jan 30, 2024

Choose a reason for hiding this comment

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

I went with the $1 syntax as my assumption is that it's less overhead compared to creating a replacement function.

} from '@vanilla-extract-private/test-helpers';

import test from './fixture';
import { webpack as testCases } from './testCases';
Copy link
Contributor Author

@askoufis askoufis Jan 30, 2024

Choose a reason for hiding this comment

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

Chose to restrict this fixture to only run using webpack.

@askoufis askoufis merged commit f0c3be9 into master Feb 1, 2024
11 checks passed
@askoufis askoufis deleted the webpack-square-brackets-fix branch February 1, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@vanilla-extract/webpack-plugin] Error: Didn't get a result from child compiler
3 participants