-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Detecting server imports on the client #2442
Conversation
Signed-off-by: Mihovil Ilakovac <[email protected]>
Signed-off-by: Mihovil Ilakovac <[email protected]>
moduleName: string; | ||
} | ||
|
||
const importStatementRegex = /\s*import\s+(?:(?:[\w*\s{},]*)\s+from\s+)?(['"`])([^'"`]+)\1\s*/g; |
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 didn't dive deep, just wanted to check, since this is regex, is there a chance of false positives? If so, in which situations, and are we ok with that? If not, can we do it better?
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.
Yes, could we maybe parse the import and do something with its AST (I'm not sure how powerful these plugins are, just throwing out ideas)?
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.
Vite doesn't expose AST information to the plugins. One the popular plugins unplugin-ast uses the Babel parser under the hood to parse files into AST.
I didn't want to go down this route due to potential performance issues, but I have to admit that I haven't timed anything. I just assumed that regexes will be good enough.
I went with a good enough approach because we are not catch all server imports anyways. We are only focusing on wasp/server/*
modules (for which we know they shouldn't be imported on the client). But... we don't catch stuff like some-node-dep
.
I will explore the AST approach and see how it works and report back. I'll also explore the JS imports syntax and see if our regex matches all the cases.
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.
It turns out we don't need to parse or use regexes at all. @sodic pointed me in the right direction, there is a different plugin hook called resolveId
which is called when Vite wants to resolve an import (so it already parsed everything) and we can decide if we want to raise a meaningful error if we notice it's a wasp/server/*
import :)
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 love the change, but have some reserverations about the implementation.
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
moduleName: string; | ||
} | ||
|
||
const importStatementRegex = /\s*import\s+(?:(?:[\w*\s{},]*)\s+from\s+)?(['"`])([^'"`]+)\1\s*/g; |
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.
Yes, could we maybe parse the import and do something with its AST (I'm not sure how powerful these plugins are, just throwing out ideas)?
Signed-off-by: Mihovil Ilakovac <[email protected]>
Signed-off-by: Mihovil Ilakovac <[email protected]>
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 like the improvements, but still have a few more ideas. Lemme kno what u think :)
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Mihovil Ilakovac <[email protected]>
Signed-off-by: Mihovil Ilakovac <[email protected]>
40099d4
to
2f93d7e
Compare
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.
Niceara.
No more blockers, just some improvements. Merge when ready.
waspc/data/Generator/templates/react-app/vite/detectServerImports.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Mihovil Ilakovac <[email protected]>
We want to detect imports from
wasp/server/*
in users' client code because we know it's incorrect. It's better to give users a nice error message than users receiving some (often hard to understand) runtime error message.This is a draft initial implementation, I'll iterate on this a bit more.
Closes #2067