Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
UpdateJsDelivrEsmResolver::IMPORT_REGEX to support dynamic imports#59858
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedFeb 26, 2025
Hey! Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "6.4". Cheers! Carsonbot |
Hi@natepage,thank you for taking the time to work on this! I'm gonna try it this afternoon on a real project to check if this change
PS: I'm not sure this should be considered a bug fix to be honest. |
JsDelivrEsmResolver::IMPORT_REGEX to support dynamic imports| @@ -28,7 +28,7 @@ final class JsDelivrEsmResolver implements PackageResolverInterface | |||
| public const URL_PATTERN_DIST = self::URL_PATTERN_DIST_CSS.'/+esm'; | |||
| public const URL_PATTERN_ENTRYPOINT = 'https://data.jsdelivr.com/v1/packages/npm/%s@%s/entrypoints'; | |||
| public const IMPORT_REGEX = '#(?:import\s*(?:[\w$]+,)?(?:(?:\{[^}]*\}|[\w$]+|\*\s*as\s+[\w$]+)\s*\bfrom\s*)?|export\s*(?:\{[^}]*\}|\*)\s*from\s*)("/npm/((?:@[^/]+/)?[^@]+?)(?:@([^/]+))?((?:/[^/]+)*?)/\+esm")#'; | |||
| public const IMPORT_REGEX = '#(?:import\s*(?:[\w$]+,)?(?:(?:\{[^}]*\}|[\w$]+|\*\s*as\s+[\w$]+)\s*\bfrom\s*)?|export\s*(?:\{[^}]*\}|\*)\s*from\s*|await\simport\()("/npm/((?:@[^/]+/)?[^@]+?)(?:@([^/]+))?((?:/[^/]+)*?)/\+esm")(?:\)*)#'; | |||
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.
what if some code stores the promise of the dynamic import instead of awaiting it immediately ?
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.
You're right.. PR / doc should state only this direct syntax works.
Still, this will allow AM to handle multiple libraries like Mermaid so i'm not sure we need to handle every case.
Is this adding the dynamically imported module in the list of dependencies of that module in the AssetMapper representation ? this would be wrong as it would break any lazy-loading done through dynamic imports. |
I tested with mermaid. 115 new entries in importmap.php115newitems (mermaid, dayjs, khroma, dompurify, @iconify/utils, @braintree/sanitize-url, d3, lodash-es/memoize.js, lodash-es/merge.js, marked, ts-dedent, roughjs, stylis, lodash-es/isEmpty.js, katex, mermaid/dist/chunks/mermaid.core/dagre-4EVJKHTY.mjs, mermaid/dist/chunks/mermaid.core/c4Diagram-6F5ED5ID.mjs, mermaid/dist/chunks/mermaid.core/flowDiagram-7ASYPVHJ.mjs, mermaid/dist/chunks/mermaid.core/erDiagram-6RL3IURR.mjs, mermaid/dist/chunks/mermaid.core/gitGraphDiagram-NRZ2UAAF.mjs, mermaid/dist/chunks/mermaid.core/ganttDiagram-NTVNEXSI.mjs, mermaid/dist/chunks/mermaid.core/infoDiagram-A4XQUW5V.mjs, mermaid/dist/chunks/mermaid.core/pieDiagram-YF2LJOPJ.mjs, mermaid/dist/chunks/mermaid.core/quadrantDiagram-OS5C2QUG.mjs, mermaid/dist/chunks/mermaid.core/xychartDiagram-6QU3TZC5.mjs, mermaid/dist/chunks/mermaid.core/requirementDiagram-MIRIMTAZ.mjs, mermaid/dist/chunks/mermaid.core/sequenceDiagram-G6AWOVSC.mjs, mermaid/dist/chunks/mermaid.core/classDiagram-LNE6IOMH.mjs, mermaid/dist/chunks/mermaid.core/classDiagram-v2-MQ7JQ4JX.mjs, mermaid/dist/chunks/mermaid.core/stateDiagram-MAYHULR4.mjs, mermaid/dist/chunks/mermaid.core/stateDiagram-v2-4JROLMXI.mjs, mermaid/dist/chunks/mermaid.core/journeyDiagram-G5WM74LC.mjs, mermaid/dist/chunks/mermaid.core/timeline-definition-U7ZMHBDA.mjs, mermaid/dist/chunks/mermaid.core/mindmap-definition-GWI6TPTV.mjs, mermaid/dist/chunks/mermaid.core/kanban-definition-QRCXZQQD.mjs, mermaid/dist/chunks/mermaid.core/sankeyDiagram-Y46BX6SQ.mjs, mermaid/dist/chunks/mermaid.core/diagram-QW4FP2JN.mjs, mermaid/dist/chunks/mermaid.core/blockDiagram-ZHA2E4KO.mjs, mermaid/dist/chunks/mermaid.core/architectureDiagram-UYN6MBPD.mjs, debug, d3-array, d3-axis, d3-brush, d3-chord, d3-color, d3-contour, d3-delaunay, d3-dispatch, d3-drag, d3-dsv, d3-ease, d3-fetch, d3-force, d3-format, d3-geo, d3-hierarchy, d3-interpolate, d3-path, d3-polygon, d3-quadtree, d3-random, d3-scale, d3-scale-chromatic, d3-selection, d3-shape, d3-time, d3-time-format, d3-timer, d3-transition, d3-zoom, dagre-d3-es/src/dagre/index.js, dagre-d3-es/src/graphlib/json.js, dagre-d3-es/src/graphlib/index.js, uuid, @mermaid-js/parser, dayjs/plugin/isoWeek.js, dayjs/plugin/customParseFormat.js, dayjs/plugin/advancedFormat.js, cytoscape, cytoscape-cose-bilkent, d3-sankey, lodash-es/clone.js, cytoscape-fcose, ms, internmap, delaunator, lodash-es, langium, @mermaid-js/parser/dist/chunks/mermaid-parser.core/info-46DW6VJ7.mjs, @mermaid-js/parser/dist/chunks/mermaid-parser.core/packet-W2GHVCYJ.mjs, @mermaid-js/parser/dist/chunks/mermaid-parser.core/pie-BEWT4RHE.mjs, @mermaid-js/parser/dist/chunks/mermaid-parser.core/architecture-I3QFYML2.mjs, @mermaid-js/parser/dist/chunks/mermaid-parser.core/gitGraph-YCYPL57B.mjs, cose-base, robust-predicates, @chevrotain/regexp-to-ast, chevrotain, chevrotain-allstar, vscode-languageserver-types, vscode-jsonrpc/lib/common/cancellation.js, vscode-languageserver-textdocument, vscode-uri, vscode-jsonrpc/lib/common/events.js, layout-base, @chevrotain/utils, @chevrotain/gast, @chevrotain/cst-dts-gen, lodash-es/map.js, lodash-es/filter.js, lodash-es/min.js, lodash-es/flatMap.js, lodash-es/uniqBy.js, lodash-es/flatten.js, lodash-es/forEach.js, lodash-es/reduce.js) added to the importmap.php! It also doubled the generated But... it worked perfectly, and i can use mermaid with no problem. |
smnandre commentedFeb 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
If i add a import{Controller}from'@hotwired/stimulus';importmermaidfrom'mermaid';/* stimulusFetch: 'lazy' */exportdefaultclassextendsController{asyncconnect(){mermaid.initialize({startOnLoad:false,securityLevel:'loose',theme:'forest',themeVariables:{primaryColor:'#ffc107',}});awaitmermaid.run({querySelector:'.mermaid',});}} (code taken from the original issue#53145 - added the lazy part ) |
Looks fine to me! |
JsDelivrEsmResolver::IMPORT_REGEX to support dynamic importsJsDelivrEsmResolver::IMPORT_REGEX to support dynamic importsThank you@natepage. |
3df7170 intosymfony:6.4Uh oh!
There was an error while loading.Please reload this page.
AYABULELA77 left a comment
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.
Good
AYABULELA77 left a comment
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 fix allows the JsDelivrEsmResolver to resolve dependencies from dynamic imports.