Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[AssetMapper] Fixing bug where a circular exception could be thrown while making error message#51345
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[AssetMapper] Fixing bug where a circular exception could be thrown while making error message#51345
Uh oh!
There was an error while loading.Please reload this page.
Conversation
51f3788 tod5823b8Compare| } | ||
| }); | ||
| $asset =newMappedAsset('html.js','/path/to/app.js'); |
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 there's a typo here, shouldn't it be'htmx.js'?
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.
Yup - it doesn't matter for the test, but definitely wasn't my intention. Fixing it - thanks :)
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.
Well, withhtml.js, the test would not trigger a circular error, and so would not prevent regressions. So it definitely matters.
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.
No - the circular reference is triggered a bit manually on the mock. So you're correct in a real-world example, but in this case, this part doesn't matter (creating a test that resulted in a natural, real-world circular reference was going to be a lot more work as we need a real AssetMapper, MappedAssetFactory, etc.
…hile making error message
nicolas-grekas commentedAug 14, 2023
Thank you@weaverryan. |
a9305a9 to63b9635Compare
AssetMapper's
JavaScriptImportPathCompilerparses import statements. That process is imperfect and will over-match in some cases (e.g. matchingimport()that is commented-out). but that's not a huge issue: any matches are simply added to the importmap and matches for not-found-files are ignored.However, in#51291, we hit a spot where, while trying to improve the log message (
Try adding ".js" to the end of the import - i.e. "%s.js"), we triggered a circular exception. This PR suppresses that.We may need to improve the parsing logic later to handle more edge-cases, but we'll handle those if/when they come up.
Cheers!