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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: 馃悰 define plugin not ignore file names #6340
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to partially fix #6340. Another case mentioned was with:
import __SOME_VAR__ from "./something"
and
export { __SOME_VAR__ }
These are rare cases though which can be handled in other PRs. Side note: I'm hoping we're not re-inventing @rollup/plugin-replace
here 馃槄
// Do not allow preceding '.', but do allow preceding '...' for spread operations | ||
'(?<!(?<!\\.\\.)\\.)\\b(' + | ||
Object.keys(replacements) | ||
.map((str) => { | ||
return str.replace(/[-[\]/{}()*+?.\\^$|]/g, '\\$&') | ||
}) | ||
.join('|') + | ||
// prevent trailing assignments | ||
')\\b(?!\\s*?=[^=])', | ||
`(?<!${characters})(?<!(?<!\\.\\.)\\.)\\b(${replacementsStr})\\b(?!${characters})(?!\\s*?=[^=])`, | ||
// prevent trailing assignments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try to split this regex to multiple lines so the comments match, and would be more readable too.
@@ -59,16 +59,18 @@ export function definePlugin(config: ResolvedConfig): Plugin { | |||
...processEnv | |||
} | |||
|
|||
const characters = '[\'"`\\/]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const characters = '[\'"`\\/]' | |
const characters = '[\'"`\\/_-]' |
For example, the following code
import * from "xxxx/xx(_|-)PATH
In my opinion, rare cases can only be solved by ast, but the performance will be reduced, so we can only consider most cases馃槅 |
You're right. I'm trying to think of ways to solve those cases, and there would always be edge cases around it unless we have the AST. I'd agree that we should keep it simple then and don't handle those. To be fair, also handling |
You're right, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting my approval as the code looks good to me. But my question still stands that whether we want to cover this extra edge case.
This PR ssems to fix some weird behavior I highlighted in this discussion: #5725 Maybe update the define docs ( |
We can discuss this one in the next team meeting, but as you are already discussing, we are going to end up with a never-ending game of updating the regex and fixing "bugs". The docs state that you should use unique values like |
@1247748612 we talked about this PR today and we decided that we should merge this fix. Thanks for looking into the issue. We should wait until the 2.9 beta window (probably starting next week) so we get some extra testing. Would you check the conflicts in the PR? |
Thanks to the team for the discussion on this pr, I have resolved the conflict. I may also need to add some test cases for this pr. |
Thanks again. I think the added test cases are enough to merge it but welcome to add more cases and improve the suite |
Co-authored-by: patak-dev <matias.capeletto@gmail.com>
@1247748612 we are reverting this PR as it causing regressions in Svelte and Vitest. We could work in another PR to implement this feature, but we should see how to simplify the regex and add more tests before moving forward. Thanks again for your work exploring this solution. |
I don't know why this is causing this, I'll try to fix it and submit the pr again after simplifying the regex. Thanks you again. |
Description
fixes: #6295
When we use import * as from "CONSTANTS" or export * from "xxxx/CONSTANTS" statements, define plugin will replace CONSTANTS, This behavior is incorrect.
I add some boundary processing to solve this problem, it is not allowed to replace variables when there are "double quotes, single quotes, back quotes, slashes" in front of the CONSTANTS.
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).