Skip to content

fix: move importmap in front of modulepreload in graphiql template#1327

Open
mantomas wants to merge 3 commits into
mirumee:mainfrom
mantomas:fix/import-map-position
Open

fix: move importmap in front of modulepreload in graphiql template#1327
mantomas wants to merge 3 commits into
mirumee:mainfrom
mantomas:fix/import-map-position

Conversation

@mantomas

@mantomas mantomas commented Jun 5, 2026

Copy link
Copy Markdown

importmap after modulepreload causes Import maps are not allowed after a module load or preload has started. in Firefox and the Graphiql playground won't load (at least in our app as mentioned in #1326) - rendering only: Loading Ariadne GraphQL…

Moving the importmap up solved the issue on our side and after reading about it I think this is more correct order and it does not reduce the performance gain from modulepreload.

Summary by CodeRabbit

  • Refactor
    • Adjusted resource loading order in the GraphiQL UI to improve asset loading and page wiring, which can reduce load glitches and improve responsiveness.
  • Tests
    • Updated HTML snapshots to match the revised page structure and resource ordering.

Greptile Summary

This PR fixes a Firefox-specific breakage in the GraphiQL playground by reordering HTML resource hints so the importmap script appears before any modulepreload or stylesheet links. The HTML spec requires import maps to be processed before module loading begins, and Firefox enforces this strictly (throwing "Import maps are not allowed after a module load or preload has started"), while Chrome is more lenient.

  • graphiql.html: <script type="importmap"> is moved above the stylesheet <link> tags and all <link rel="modulepreload"> entries, which is the spec-compliant order and resolves the Firefox blank-page regression.
  • executable_schema.py: Unrelated minor typing improvement — new_bindables gets an explicit annotation and cast() is applied on the list-extend call to satisfy static type checkers.
  • Snapshots: All three test snapshot files are updated to match the new HTML output order.

Confidence Score: 5/5

Safe to merge — the change brings the template into spec-compliant ordering and fixes a real browser crash with no risk of regression on other browsers.

The reordering is a straightforward, targeted fix: moving the importmap before modulepreload is what the HTML spec requires, Chrome already tolerated the old order, and Firefox now works correctly. All snapshots are updated. The unrelated typing change in executable_schema.py is mechanical and introduces no behavioral difference.

No files require special attention.

Important Files Changed

Filename Overview
ariadne/explorer/templates/graphiql.html Moves importmap script before stylesheet and modulepreload links, fixing Firefox's strict enforcement that import maps must precede any module loading.
ariadne/executable_schema.py Adds an explicit type annotation to new_bindables and wraps the list-extend call with cast to satisfy static type checkers; no behavioral change.
tests/asgi/snapshots/test_explorer.ambr Snapshot updated to reflect the new resource-loading order (importmap first, then stylesheets and modulepreloads).
tests/explorers/snapshots/test_explorers.ambr Snapshot updated consistently with the new HTML loading order across all explorer test variants.
tests/wsgi/snapshots/test_explorer.ambr WSGI snapshot updated to reflect the reordered importmap placement, matching the template change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["style (inline CSS)"] --> B

    subgraph BEFORE ["Before (Firefox broken)"]
        B1["link rel=stylesheet (graphiql CSS)"]
        B2["link rel=modulepreload (react, graphiql, ...)"] 
        B3["script type=importmap"]
        B4["script type=module"]
        B1 --> B2 --> B3 --> B4
    end

    subgraph AFTER ["After (spec-compliant, Firefox fixed)"]
        C1["script type=importmap - moved first"]
        C2["link rel=stylesheet (graphiql CSS)"]
        C3["link rel=modulepreload (react, graphiql, ...)"]
        C4["script type=module"]
        C1 --> C2 --> C3 --> C4
    end

    A --> BEFORE
    A --> AFTER
Loading

Reviews (2): Last reviewed commit: "fix: update test data after change and a..." | Re-trigger Greptile

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Reorders GraphiQL-related <link> tags (CSS and modulepreload) to appear after the <script type="importmap"> block in the HTML head; updates snapshots and adds a typing cast in flatten_bindables.

Changes

GraphiQL Head Resource Loading Order

Layer / File(s) Summary
Modulepreload and CSS link tag reordering
ariadne/explorer/templates/graphiql.html, tests/asgi/__snapshots__/test_explorer.ambr, tests/explorers/__snapshots__/test_explorers.ambr, tests/wsgi/__snapshots__/test_explorer.ambr
Relocate GraphiQL CSS, optional explorer plugin CSS, and React/ReactDOM/GraphiQL rel="modulepreload" <link> tags to after the <script type="importmap"> block; snapshots updated to match.

Typing adjustment in schema utilities

Layer / File(s) Summary
Typed flatten_bindables update
ariadne/executable_schema.py
Add cast to typing imports and use an explicitly typed new_bindables list with cast(...) when extending nested bindable lists; no runtime behavior change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Possibly related PRs

  • mirumee/ariadne#1306: Related GraphiQL head markup rework and importmap/modulepreload relocation.
  • mirumee/ariadne#1293: Related edits to GraphiQL head assets (SRI/crossorigin additions and similar template changes).

Suggested reviewers

  • DamianCzajkowski

Poem

I’m a rabbit in the repo, hopping through the head,
Moving importmaps first so preloads can be fed,
Styles and modules line up neat and spry,
🐇✨ GraphiQL wakes with order—watch it fly!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: move importmap in front of modulepreload in graphiql template' directly and specifically summarizes the main change: relocating the importmap script tag before modulepreload link tags in the GraphiQL template to resolve a Firefox loading issue.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.41%. Comparing base (53496de) to head (eec972b).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1327      +/-   ##
==========================================
- Coverage   97.51%   97.41%   -0.10%     
==========================================
  Files         140      140              
  Lines       10449    10449              
==========================================
- Hits        10189    10179      -10     
- Misses        260      270      +10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mantomas

mantomas commented Jun 5, 2026

Copy link
Copy Markdown
Author

Looks like temp issue with the test, all passed locally before push.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant