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.

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

Script type module #1682

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Script type module #1682

wants to merge 7 commits into from

Conversation

jmorag
Copy link

@jmorag jmorag commented Jun 24, 2020

WIP #1681 yesodweb/shakespeare#253
Adding javascript modules via jsModule/juliusModule/juliusModuleFile etc. works, but I'm a little stuck on getting widgetFile to work with this. Is adding the extra field to GWData the best way to go about that or is there some other way that would involve less code duplication? Once that's figured out I'll bump the version number and add some descriptions in the haddocks.

@snoyberg
Copy link
Member

These two PRs seem like more work than I was expecting for this feature. Correct me if I'm wrong, but you can achieve what you're looking for right now with Hamlet as-is, via something like:

let moduleContent = [julius|...|]
toWidget [hamlet|
  <script type="module">#{renderJavascript moduleContent}
|]

Am I misunderstanding the problem?

@jmorag
Copy link
Author

jmorag commented Jun 24, 2020

You could probably do that, but I'm not sure how to get something like

defaultLayout $ do
  [jsModule|
import foo from 'bar';
foo();
|]

to work as a widget without a newtype for the ToWidget instances.

@snoyberg
Copy link
Member

OK, but you've focused on a very specific API design that requires significant changes to two packages, when it seems like there are simpler alternatives available. I'm trying to start off from understanding the actual goals here. Instead, you're showing me a piece of code you want to write and asserting that it requires a ToWidget instance, none of which may be true.

@jmorag
Copy link
Author

jmorag commented Jun 26, 2020

You're right. This large an API change is almost certainly not the right way to go about this. I probably should have thought it through a bit more before opening the PR...

In any case, after more thought, it seems like the simplest way to go about this would be to just make the new quasiquoters jsModule and jsModuleFile[Reload] which call renderJavascript and add the enclosing <script> tags. I imagine this would only require touching shakespeare and not yesod proper, except for optionally adding the 'mjulius' extension to the default widgetFile settings. Does that seem like a reasonable approach? Something like

-jsModuleSettings :: Q ShakespeareSettings
-jsModuleSettings = do
-  toJExp <- [|toJavascript|]
-  wrapExp <- [|Javascript . (\js -> "<script type=\"module\">" <> js <> "</script>")|]
-  unWrapExp <- [|unJavascript|]
-  asJavascriptUrl' <- [|asJavascriptUrl|]
-  return $ defaultShakespeareSettings { toBuilder = toJExp
-  , wrap = wrapExp
-  , unwrap = unWrapExp
-  , modifyFinalValue = Just asJavascriptUrl'
-  }

EDIT: something like this is probably more appropriate

asJsModuleUrl :: JavascriptUrl url -> HtmlUrl url
asJsModuleUrl = fmap
  (\j -> toHtml $ "<script type=\"module\">\n" <> unJavascript j <> "\n</script>")

jsModuleSettings :: Q ShakespeareSettings
jsModuleSettings = do
  settings <- javascriptSettings
  asUrl' <- [|asJsModuleUrl|]
  return $ settings { modifyFinalValue = Just asUrl' }

Thank you for your quick responses so far and for taking the time to look this over. I really appreciate your willingness to add this feature and your work on Yesod as a whole.

@snoyberg
Copy link
Member

Sure, my pleasure. I'm going to admit that it's been a while since I've looked at this TH code myself, but it looks like what you're providing in your last comment makes a lot of sense. Have you tested if that will work for your use case?

I'm not sure if this belongs in Shakespeare or Yesod itself, I could see arguments going either way, and I'm not particularly opinionated.

For widgetFile settings: it's been a long time since we modified the defaults there. I think it would make more sense to start off with documentation or a helper function to show how to modify the defaults to support this alternatives mjulius syntax, and then consider changing the defaults later. Does that make sense?

@jmorag
Copy link
Author

jmorag commented Jun 29, 2020

For having an inline toWidget [jsModule|import whatever from 'somewhere'], having this in Shakespeare seems fine. I just pushed the working implementation to the PR on that repo. It seems like it doesn't play very nicely with file reloading though, as non-module and module files get concatenated, causing errors. Re: documentation, the wiki already has a page on adding coffeescript to the widget file settings which generalizes very obviously to adding mjulius files, but I'm happy to add an extra note there if you think it will be helpful. Leaving the defaults as is for now makes total sense.

The argument for having this functionality in yesod proper would be for it to play better with widgetToPageContent and the defaults for addStaticContent. As far as I can tell, getting that functionality would require updating GWData's gwdJavascript field to include information about whether the javascript should be emitted with the module tag or not. The argument against is that in all likelihood, it would be a large change whereas if the implementation is solely in shakespeare, the diff is quite small and contained.

@snoyberg
Copy link
Member

as non-module and module files get concatenated, causing errors

I'm not sure how that's playing out in practice. I would have thought each module file is essentially turning into its own HTML element which is being treated separately. But like I said, I haven't worked on the codebase in a while.

@whirlicote
Copy link

The example you provided did not compile for me. I got it working with something like this:

{-# LANGUAGE QuasiQuotes #-}

module YesodJavaScriptModule where

import Yesod
import Text.Julius
import Text.Blaze

import qualified Data.Text.Lazy as LT
import qualified Data.Text as T


data JavaScriptInjection
  = JavaScriptInjection LT.Text

instance ToMarkup JavaScriptInjection where
    toMarkup (JavaScriptInjection x) = preEscapedLazyText {-lazyText-} x
    {-# INLINE toMarkup #-}
    preEscapedToMarkup (JavaScriptInjection x) = preEscapedLazyText x
    {-# INLINE preEscapedToMarkup #-}

toJavaScriptModule
  :: MonadWidget m
  => ((Route (HandlerSite m) -> [(T.Text, T.Text)] -> T.Text) -> Javascript)
  -> m ()
toJavaScriptModule javaScript = do
  params <- getUrlRenderParams
  let moduleContent = javaScript params
  let stuff = JavaScriptInjection . renderJavascript
  toWidget
    [hamlet|
      <script type="module">#{ (stuff moduleContent) }
    |]


toJavaScriptImportMap
  :: MonadWidget m
  =>
    ( (Route (HandlerSite m)
    -> [(T.Text, T.Text)]
    -> T.Text)
    -> Javascript
    )
  -> m ()

toJavaScriptImportMap importMap = do
  params <- getUrlRenderParams
  let moduleContent = importMap params
  let stuff = JavaScriptInjection . renderJavascript
  toWidget
    [hamlet|
      <script type="importmap">#{ (stuff moduleContent) }
    |]

and then using it like this:

  toJavaScriptModule $ mconcat
    [ [julius| console.log("code 1") |]
    , [julius| console.log("code 2") |]
    , [julius| console.log("code 3") |]
    ]

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.

3 participants