-
Notifications
You must be signed in to change notification settings - Fork 916
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
use regex for detecting import.meta #1371
use regex for detecting import.meta #1371
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/3ochcxrzx |
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.
LGTM with 1 comment. yarn build && yarn test -u
should fix snapshots automatically.
@@ -30,7 +30,7 @@ export function wrapImportMeta({ | |||
env: boolean; | |||
config: SnowpackConfig; | |||
}) { | |||
if (!code.includes('import.meta')) { | |||
if (!/import\s*\.\s*meta/.test(code)) { |
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 is a pretty hot code path, can you move the regex creation outside of the function?
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.
done
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.
LGTM! Will merge once tests pass
Changes
The code that detects
import.meta
, in order to inject the HMR client etc, is using a naivecode.includes('import.meta')
check. This updates it to use a more robust regular expression, that can catch things likewhich could be injected by tools like Prettier.
Testing
I changed one of the tests that uses
import.meta.hot
, but wasn't sure how to update the snapshot, so it's currently failing.Docs
No docs, bug fix only