Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ Feature – Allow script tags for CDN links and polyfills #49

Closed
3 tasks
ryan-haskell opened this issue Oct 13, 2022 · 5 comments · Fixed by #55
Closed
3 tasks

✨ Feature – Allow script tags for CDN links and polyfills #49

ryan-haskell opened this issue Oct 13, 2022 · 5 comments · Fixed by #55
Labels
Feature Adding a new capability or user-facing feature

Comments

@ryan-haskell
Copy link
Contributor

ryan-haskell commented Oct 13, 2022 •
edited

Problem

Earlier in Discord, a user (shout-out to bees#5238) wanted to use Sentry to handle error tracking. To get production error-reporting set up with a CDN link, they need the ability to insert <script> tags into Elm Land's single HTML file.

Unfortunately, the elm-land.json configuration file doesn't support that today!

Solution

  • Add an empty "script": [] field to the default elm-land.json configuration file
  • Use that field to generate <script> tags at the very end of the HTML <head> tag
  • We should use the API popularized by vue-meta

Example

Let's say I wanted to add the following Sentry CDN script tag to my Elm Land application:

<script
  src="https://browser.sentry-cdn.com/7.15.0/bundle.min.js"
  integrity="sha384-PqiX8ahlE6YeAoIVKdKCVIrGFo9vlS8nYD9v+/5Len/XiUpZkii7ehjsDMsNnXKo"
  crossorigin="anonymous"
></script>

Here's how I would update the elm-land.json file:

{
  "app": {
    "env": [],
    "html": {
      "attributes": {
        "html": { "lang": "en" },
        "head": {},
        "body": {}
      },
      "title": "My Elm Land App",
      "meta": [
        { "charset": "UTF-8" },
        { "http-equiv": "X-UA-Compatible", "content": "IE=edge" },
        { "name": "viewport", "content": "width=device-width, initial-scale=1.0" }
      ],
      "link": [],
      "script": [
        { 
          "src": "https://browser.sentry-cdn.com/7.15.0/bundle.min.js",
          "integrity": "sha384-PqiX8ahlE6YeAoIVKdKCVIrGFo9vlS8nYD9v+/5Len/XiUpZkii7ehjsDMsNnXKo",
          "crossorigin": "anonymous"
        }
      ]
    }
  }
}

In my browser, I'll see this HTML when I inspect the page source:

<!DOCTYPE html>
<html lang="en">
  <head>
    <title>My Elm Land App</title>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <script src="https://browser.sentry-cdn.com/7.15.0/bundle.min.js" integrity="sha384-PqiX8ahlE6YeAoIVKdKCVIrGFo9vlS8nYD9v+/5Len/XiUpZkii7ehjsDMsNnXKo" crossorigin="anonymous"></script>
    <script type="module" crossorigin src="/assets/index.9acd740b.js"></script>
  </head>
  <body>
    <div id="app"></div>
  </body>
</html>
@ryan-haskell ryan-haskell added @elm-land/app Feature Adding a new capability or user-facing feature labels Oct 13, 2022
@ryan-haskell
Copy link
Contributor Author

🧙‍♂️ For context, here's the part of the Elm Land codebase that is responsible for generating the index.html file:

elm-land/projects/cli/src/effects.js

Lines 361 to 368 in 5d3ecbc

let htmlContent = `<!DOCTYPE html>
<html${htmlAttributes}>
<head${headAttributes}>${headTags}</head>
<body${bodyAttributes}>
<div id="app"></div>
<script type="module" src="./main.js"></script>
</body>
</html>`

@thomasin
Copy link
Contributor

thomasin commented Nov 7, 2022

@ryannhg So I have run into issues but I am also having second thoughts about the idea in the first place.
First of all, Vite can't seem to find files not moved into the .elm-land/server folder, which is complicated when the user is giving us a bunch of files. I initially tried to put them in static, but then it didn't want to follow imports. Never used Vite before so might be missing something (?)

But in testing I also noticed that process.env is replaced by import.meta.env. So, for people to use their own JS files they would have to buy in to Vite which is I think what you were wanting to avoid by using elm-land.json in the first place.

I don't know what the right answer is, maybe only processing script tags that link to a file in static (so no imports) or URLs, and then adding a beforeReady function in interop.js for setup work. But that's just a thought.
I didn't do much actual work it was mostly testing but this is what I did do main...thomasin:elm-land:feature/allow-script-tags

@ryan-haskell
Copy link
Contributor Author

maybe only processing script tags that link to a file in static (so no imports) or URLs

This plan makes sense to me– I thought the initial goal was to link to a CDN URL, rather than worrying about the NPM module.

In the example shown above, we only support URLs from an existing CDN and have those be inserted into the HTML file at build time.

If the scripts feature worked exactly like the existing link feature, I'd be more than happy to merge it in!

Would this simple solution address the initial problem you ran into when setting up Sentry?

@thomasin
Copy link
Contributor

maybe only processing script tags that link to a file in static (so no imports) or URLs

This plan makes sense to me– I thought the initial goal was to link to a CDN URL, rather than worrying about the NPM module.

In the example shown above, we only support URLs from an existing CDN and have those be inserted into the HTML file at build time.

If the scripts feature worked exactly like the existing link feature, I'd be more than happy to merge it in!

Would this simple solution address the initial problem you ran into when setting up Sentry?

Oh I see (: We don't use any CDNs but they shouldn't be hard to switch to. I have a couple pieces of code that are run before the app initialises, but I don't see any reason they can't be moved & either way allowing for URLs is a good first step.

The code I linked above does work exactly the same, but it doesn't do any verification on attributes or that the script source is a URL. Is that alright?

@ryan-haskell
Copy link
Contributor Author

Hey @thomasin, the changes in your branch look great to me– open a PR and I'll merge it in!

@thomasin thomasin mentioned this issue Nov 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Adding a new capability or user-facing feature
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add script tags to the end of the generated index.html thomasin/elm-land
2 participants
@ryan-haskell @thomasin