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

define doesn't work with keys starting with $ #5956

Closed
7 tasks done
otakustay opened this issue Dec 3, 2021 · 3 comments · Fixed by #5972
Closed
7 tasks done

define doesn't work with keys starting with $ #5956

otakustay opened this issue Dec 3, 2021 · 3 comments · Fixed by #5972
Labels
enhancement New feature or request has workaround p2-nice-to-have Not breaking anything but nice to have (priority)

Comments

@otakustay
Copy link
Contributor

Describe the bug

define config doesn't work with keys starting with $ like $foo.bar

Reproduction

  1. From vite vanilla template

  2. Add following to vite.config.ts:

    import { defineConfig } from 'vite'
    
    // https://vitejs.dev/config/
    export default defineConfig({
      define: {
        '$build.foo': '"bar"',
        'dev.foo': '"bar"',
      },
    })
  3. change main.js to following:

    console.log($build.foo);
    console.log(dev.foo);
  4. Run vite build

  5. Open dist/assets/index.xxx.js and find console.log calls

We expect both console.log arguments to be replaced to "bar", however the actual build asset is (formatted):

console.log($build.foo);
console.log("bar");

Keys starts with $ is not replaced.

By digging a little deeper, we simplified the regex pattern generated to this:

const regex = /\b(\$build\.foo)\b/g
regex.test('$build.foo'); // false

Without $ prefix regex works as expected:

const c = /\b(build\.foo)\b/g;
console.log(c.test('build.foo')); // true

Maybe this is an issue with the regex generation.

System Info

System:
    OS: macOS 12.0.1
    CPU: (8) arm64 Apple M1
    Memory: 162.14 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.13.0 - /var/folders/9v/sx18bwn925b3f2rp2jfg952w0000gn/T/fnm_multishells/5569_1638539624158/bin/node
    Yarn: 1.22.17 - /var/folders/9v/sx18bwn925b3f2rp2jfg952w0000gn/T/fnm_multishells/5569_1638539624158/bin/yarn
    npm: 8.1.0 - /var/folders/9v/sx18bwn925b3f2rp2jfg952w0000gn/T/fnm_multishells/5569_1638539624158/bin/npm
  Browsers:
    Chrome: 96.0.4664.55
    Safari: 15.1
  npmPackages:
    vite: ^2.6.4 => 2.6.14

Used Package Manager

yarn

Logs

vite:config bundled config file loaded in 10.75ms +0ms
  vite:config using resolved config: {
  vite:config   define: { '$build.foo': '"bar"', 'dev.foo': '"bar"' },
  vite:config   build: {
  vite:config     target: [ 'es2019', 'edge88', 'firefox78', 'chrome87', 'safari13.1' ],
  vite:config     polyfillModulePreload: true,
  vite:config     outDir: 'dist',
  vite:config     assetsDir: 'assets',
  vite:config     assetsInlineLimit: 4096,
  vite:config     cssCodeSplit: true,
  vite:config     cssTarget: [ 'es2019', 'edge88', 'firefox78', 'chrome87', 'safari13.1' ],
  vite:config     sourcemap: false,
  vite:config     rollupOptions: {},
  vite:config     minify: 'esbuild',
  vite:config     terserOptions: {},
  vite:config     write: true,
  vite:config     emptyOutDir: null,
  vite:config     manifest: false,
  vite:config     lib: false,
  vite:config     ssr: false,
  vite:config     ssrManifest: false,
  vite:config     reportCompressedSize: true,
  vite:config     chunkSizeWarningLimit: 500,
  vite:config     watch: null,
  vite:config     commonjsOptions: { include: [Array], extensions: [Array] },
  vite:config     dynamicImportVarsOptions: { warnOnError: true, exclude: [Array] }
  vite:config   },
  vite:config   configFile: '/Users/otakustay/Downloads/vite-issue/vite.config.ts',
  vite:config   configFileDependencies: [ 'vite.config.ts' ],
  vite:config   inlineConfig: {
  vite:config     root: undefined,
  vite:config     base: undefined,
  vite:config     mode: undefined,
  vite:config     configFile: undefined,
  vite:config     logLevel: undefined,
  vite:config     clearScreen: undefined,
  vite:config     build: {}
  vite:config   },
  vite:config   root: '/Users/otakustay/Downloads/vite-issue',
  vite:config   base: '/',
  vite:config   resolve: { dedupe: undefined, alias: [ [Object], [Object] ] },
  vite:config   publicDir: '/Users/otakustay/Downloads/vite-issue/public',
  vite:config   cacheDir: '/Users/otakustay/Downloads/vite-issue/node_modules/.vite',
  vite:config   command: 'build',
  vite:config   mode: 'production',
  vite:config   isProduction: true,
  vite:config   plugins: [
  vite:config     'alias',
  vite:config     'vite:modulepreload-polyfill',
  vite:config     'vite:resolve',
  vite:config     'vite:html-inline-script-proxy',
  vite:config     'vite:css',
  vite:config     'vite:esbuild',
  vite:config     'vite:json',
  vite:config     'vite:wasm',
  vite:config     'vite:worker',
  vite:config     'vite:asset',
  vite:config     'vite:define',
  vite:config     'vite:css-post',
  vite:config     'vite:build-html',
  vite:config     'commonjs',
  vite:config     'vite:data-uri',
  vite:config     'rollup-plugin-dynamic-import-variables',
  vite:config     'vite:asset-import-meta-url',
  vite:config     'vite:build-import-analysis',
  vite:config     'vite:esbuild-transpile',
  vite:config     'vite:reporter',
  vite:config     'vite:load-fallback'
  vite:config   ],
  vite:config   server: { fs: { strict: undefined, allow: [Array] } },
  vite:config   env: { BASE_URL: '/', MODE: 'production', DEV: false, PROD: true },
  vite:config   assetsInclude: [Function: assetsInclude],
  vite:config   logger: {
  vite:config     hasWarned: false,
  vite:config     info: [Function: info],
  vite:config     warn: [Function: warn],
  vite:config     warnOnce: [Function: warnOnce],
  vite:config     error: [Function: error],
  vite:config     clearScreen: [Function: clearScreen],
  vite:config     hasErrorLogged: [Function: hasErrorLogged]
  vite:config   },
  vite:config   createResolver: [Function: createResolver],
  vite:config   optimizeDeps: {
  vite:config     esbuildOptions: { keepNames: undefined, preserveSymlinks: undefined }
  vite:config   }
  vite:config } +2ms
vite v2.6.14 building for production...
✓ 3 modules transformed.
dist/assets/favicon.17e50649.svg   1.49 KiB
dist/index.html                    0.39 KiB
dist/assets/index.77d0d5b3.js      0.74 KiB / gzip: 0.42 KiB

Validations

@jonaskuske
Copy link
Contributor

jonaskuske commented Dec 4, 2021

I'd say this is working as intended, so this would be a feature request. (to change how define works or make it configurable)
However it might be a good idea to clarify the docs or at least link to some RegExp explanation. (or this issue?)

TL;DR:
Change your key to start and end with characters that match [a-zA-Z0-9_], not $.

From the docs:

  • Replacements are performed only when the match is surrounded by word boundaries (\b).

Because it's implemented as straightforward text replacements without any syntax analysis, we recommend using define for CONSTANTS only.

So at the start and the end of your replacement string there needs to be a word boundary in order for it to be replaced. A word boundary is a point where a non-word character meets a word character. Word characters are anything that match \w:
the letters A to Z (lowercase or uppercase), digits and underscores. ([a-zA-Z0-9_])

You usually want to have the key replaced when it is surrounded by non-word characters:

// { and }
let msg = `you are in ${_ENV_}`;

// ( and )
console.log(_ENV_);

// space and ;
let env = _ENV_;

// ...

At the same, you don't want it replaced when surrounded by word characters:

let secretKey = "sshABC_5353jf62_ENV_Pf3g_h"; // don't replace it here
const SOME_OTHER_ENV_DATA = []; // also not here!

So, your replacement key must start and end with a word character!
This way, the word boundary and therefore a replacement will happen when the key is surrounded by non-word characters, and not the other way arround.




In your case, the $ at the beginning of your replacement key is a non-word character. So this won't be replaced, as you noticed:

// `($` isn't a word boundary and doesn't match the `\b` in the RegExp
console.log($build.foo)

This however will be replaced:

// `o$` in the beginning and `o'` in the end are both word boundaries so the key is surrounded by them
console.log('hello$build.foo')

@otakustay
Copy link
Contributor Author

Thanks for explanation, I still believe this can be a feature request since the only missing character comparing to JavaScript Identifier syntax is $, and $ is quite widely used to mark some globals as "reserved by a certain framework or tool" to prevent variable name conflict.

@jonaskuske
Copy link
Contributor

jonaskuske commented Dec 4, 2021

the only missing character comparing to JavaScript Identifier syntax is $

Unfortunately not, since JS Identifiers allow Unicode letters and digits, but RegExp \b and \w only match ASCII letters/digits. (in JavaScript)
So a JS RegExp only allows A-Z while JS Identifiers also allow Ä, Ø, ß, ɵ and a ton of other characters.

Nonetheless, I agree that an exception is probably a good idea here!

Edit:

Just checked, ES2018 comes with everything we'd need to replace \b with a solution that can handle both $ and Unicode and therefore work with any valid JS Identifier – Unicode property escapes and lookbehinds, supported from Node v10 upwards. I'll submit a PR later, but since ASCII-only and non-$ \b was specced in the docs this technically is a breaking change instead of a fix, so not sure what the policy is here. I think cases where this would break something should be somewhere between super rare to non-existent so might be okay for a feature release? @sodatea

@sapphi-red sapphi-red added enhancement New feature or request has pr p2-nice-to-have Not breaking anything but nice to have (priority) and removed enhancement: pending triage labels Apr 21, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request has workaround p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants