carefully filter require arguments #138
Conversation
|
|
| // Filter invalid package calls, Architect shared + views, and Node.js builtins | ||
| let isArcShared = /^@architect(\/|\\)(shared|views)/ | ||
| called.forEach(r => { | ||
| if (typeof r === 'string') { |
There was a problem hiding this comment.
Would we expect to see !string values in there? When and under what circumstances?
There was a problem hiding this comment.
I caught this when using esbuild to preprocess lambdas. It reuses variables, like such:
let x = 123
// (use x)
x = 'lodash'
y = require(x)
And when looking for the initialisation of "x", it sees a number.
| require(requiredThing) | ||
|
|
||
| // There may be a function called require that's not the real thing | ||
| require(1234) |
There was a problem hiding this comment.
Is this a case that we really ought to support? If someone is writing Node.js code that breaks CJS, I don't see any responsibility on our part to aid that.
There was a problem hiding this comment.
I agree. Though it's entirely possible that someone created a function called require, but I won't die on that hill :)
| // When minifiers redefine variables, the original might be an invalid module string | ||
| let requiredThing = 'some thing ' | ||
| requiredThing = 'validmodule' | ||
| require(requiredThing) |
There was a problem hiding this comment.
It seems like you want to support this but I'm not seeing it appear in the tests, what was the intent here? Sorry I'm not following!
Also can we change that from 'some thing ' to something else? We already have a match in there for something and I just want to be a bit clearer. Thanks!
There was a problem hiding this comment.
It's another case of minifiers reusing variables. Yes I can change to "invalid module string "
| // Minifiers may redefine variables to save space | ||
| let missingInit | ||
| missingInit = 'ignoredmodule' | ||
| require(missingInit) |
There was a problem hiding this comment.
In general I hope we'll do our best to warn folks; in this change I don't think we've successfully warned the user about these missing requires, have we?
|
After writing this PR I noticed that I didn't need hydration after all (esbuild was doing all the work). So I completely understand if you don't want to support my use case, and I can remove some of the functionality. |
|
Up to you! Seemed like some potentially useful stuff in there, but I can't say it solves problems I've had. |
I've tried to hydrate some code I ran through esbuild (to ease typescript compilation), and I noticed some issues that came up since it minifies code. Most of them were related to esbuild (like other minifiers) reusing variables, as illustrated in the test mock I've changed. I also fixed a crash which occurred when esquery returned a VariableDeclarator without an initializer. Nice project, keep it up, and hope this helps! <3
fabiosantoscode
force-pushed
the
carefully-filter-require-arguments
branch
from
f48ceb5
to
f26f209
Compare
|
Sorry for the delay @ryanblock! I've added some warnings, and tests for those warnings. You said "some potentially useful stuff" but I'm not sure which checks you'd like for me to remove so I kept them all in, please advise. |
I've tried to hydrate some code I ran through esbuild (to ease typescript compilation), and I noticed some issues that came up since it minifies code.
Most of them were related to esbuild (like other minifiers) reusing variables, as illustrated in the test mock I've changed.
I also fixed a crash which occurred when esquery returned a VariableDeclarator without an initializer.
Nice project, keep it up, and hope this helps! <3
Thank you for helping out! ✨
We really appreciate your commitment to improving Architect
To maintain a high standard of quality in our releases, before merging every pull request we ask that you've completed the following:
masternpm itfrom the repo root)readme.md, help docs, inline docs & comments, etc.)changelog.mdPlease also be sure to completed the CLA (if you haven't already).
Learn more about contributing to Architect here.
Thanks again!