replace: Should property accessors be replaced or not?

オープン
#2,011 コメント 1 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

評価

難易度
5/5
見積もり時間
1週間以上
初心者へのやさしさ
35/100
issue の種類
バグ
明瞭さ
説明が足りない
活発さ
静か
技術スタック
javascript
領域
tooling

調査の方向性

Compare packages/replace/test/fixtures/form/dollar-sign-boundary and replace-nothing with their _config.js files and the Vitest snapshots under packages/replace/test/snapshots/form.js.md, using the linked reproduction as context. First establish which property-access and delimiter behavior is intended; done means the conflicting fixtures and snapshots consistently reflect the agreed behavior.

索引モデルが issue の本文から書いたものです。

説明

Expected Behavior

Given this test added in #1938 merged into 6.0.3:
https://github.com/rollup/plugins/blob/639f45638234c1c3fabfb13615c78bebaef89ef2/packages/replace/test/fixtures/form/dollar-sign-boundary/input.js#L17-L20
https://github.com/rollup/plugins/blob/639f45638234c1c3fabfb13615c78bebaef89ef2/packages/replace/test/fixtures/form/dollar-sign-boundary/_config.js#L2-L8
Comment strongly suggests it should produce the following replacement:

// Should be replaced - typeof window followed by dot
if ("undefined".document !== 'undefined') {
  console.log('has document');
}
Actual Behavior

The snapshot stored by vitest does not actually replace this:
https://github.com/rollup/plugins/blob/639f45638234c1c3fabfb13615c78bebaef89ef2/packages/replace/test/snapshots/form.js.md?plain=1#L43-L46

And, there is an older fixture + snapshot which directly conflicts with this interpretation, under replace-nothing:
https://github.com/rollup/plugins/blob/639f45638234c1c3fabfb13615c78bebaef89ef2/packages/replace/test/fixtures/form/replace-nothing/input.js#L1-L2
https://github.com/rollup/plugins/blob/639f45638234c1c3fabfb13615c78bebaef89ef2/packages/replace/test/fixtures/form/replace-nothing/_config.js#L2-L5
Vitest Snapshot:
https://github.com/rollup/plugins/blob/639f45638234c1c3fabfb13615c78bebaef89ef2/packages/replace/test/snapshots/form.js.md?plain=1#L76-L81

Additional Information

The older PR which introduced the replace-nothing test was #903 which had the following comment:

Personally, I believe it's dangerous to replace VERSION in the case you highlighted by default:

if (VERSION.startsWith('foo')) {
  // things
}

In the case above with the previous default to match VERSION when used with a . or nested access, you would need to know all of your dependencies' code, not to mess up when replacing.

Originally posted by @o-alexandrov in https://github.com/rollup/plugins/issues/903#issuecomment-885657501

What is the current intended behavior? I don't think these two tests can co-exist unless they're testing differently configured delimiters.

主要言語
JavaScript
スター
3.8k
フォーク
635
PR マージ指標
30日以内にマージされた PR はありません

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

rollup/plugins のほかの issue

rollup/plugins の issue をすべて見る

似ている issue

JavaScript の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。