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

assemble page and app <head> tags in the correct order #431

Closed
1 of 5 tasks
thescientist13 opened this issue Nov 8, 2020 · 4 comments · Fixed by #475
Closed
1 of 5 tasks

assemble page and app <head> tags in the correct order #431

thescientist13 opened this issue Nov 8, 2020 · 4 comments · Fixed by #475
Assignees
@thescientist13
Labels
alpha.7 bug Something isn't working CLI good first issue Good for newcomers v0.10.0
Milestone
No Bundle Feature...

Comments

@thescientist13
Copy link
Member

Type of Change

  • New Feature Request
  • Documentation / Website
  • Improvement / Suggestion
  • Bug
  • Other (please clarify below)

Summary

Currently when a page is assembled into an app template in HTML transformation, script / style / link tags from the page are not appended after all the app ones, instead they are appended after just the first one

currently its like this

<head>
   <script app-1>
   <script page-1>
   <script page-2>
   <script app-2>
</head>

it should be like this

<head>
   <script app-1>
   <script app-2>
   <script page-1>
   <script page-2>
</head>

Details

Basically this code is too naive with the regex, maybe it would be better to use HtmlParser like in rollup?

headScripts.forEach(script => {
  if (script.rawAttrs !== '') {
    appTemplateContents = appTemplateContents.replace(/<\/script>/, `
      </script>\n
      <script ${script.rawAttrs}></script>\n
    `);
  }

  if (script.rawAttrs === '') {
    appTemplateContents = appTemplateContents.replace(/<\/script>/, `
      </script>\n
      <script>
        ${script.text}
      </script>\n
    `);
  }
});

headLinks.forEach(link => {
  appTemplateContents = appTemplateContents.replace(/<\/link>/, `
    </link>\n
    <link ${link.rawAttrs}></link>\n
  `);
});

In general, this naive regexing throughout may want to be refactored. But that could be another ticket. Not sure which is faster (regex vs parser?
@thescientist13 thescientist13 added the enhancement Improve something existing (e.g. no docs, new APIs, etc) label Nov 8, 2020
@thescientist13 thescientist13 changed the title assemble page and app script / style tags in the correct order assemble page and app <head> tags in the correct order Nov 8, 2020
@hutchgrant
Copy link
Member

hutchgrant commented Nov 10, 2020 •
edited
Loading

My preference is to do this with jsdom. Also the problem is we're adding the page scripts at the same time as filling the <page-outlet>. This becomes problematic with ordering when you also have to fill the meta, importmap, and livereload. Each of which rely on other logic for when they're run. I have a branch with some success with this, but the order is still not right.

@thescientist13
Copy link
Member Author

What I had in mind for this (and anywhere we are doing for this kind of find / replace / regex) was doing more things like this from the rollup.config.js to programmatically walkthrough markup strings.

const parser = new htmlparser2.Parser({
  onopentag(name, attribs) {
    if (name === 'script' && attribs.type === 'module' && attribs.src) {
      const srcPath = attribs.src.replace('../', './');
      const scriptSrc = fs.readFileSync(path.join(userWorkspace, srcPath), 'utf-8');

      that.emitFile({
        type: 'chunk',
        id: srcPath,
        name: srcPath.split('/')[srcPath.split('/').length - 1].replace('.js', ''),
        source: scriptSrc
     });

      // console.debug('emitFile for script => ', srcPath);
   }
 }
});

I don't think we require an entire browser implementation just for this. Plus, can JSDOM even do that? I thought it was more for emulating a browser environment in JS.

Even with some generous marge, JSDOM is MBs say compared to something like htmlparser2 so that's a lot of overhead / potential overkill.

@hutchgrant
Copy link
Member

well this isnt running on the browser, 2mb on the compiling machine is nothing. Especially when you consider puppeteer also downloads chromium(~170MB Mac, ~282MB Linux, ~280MB Win). Mine was a hacky solution, similar to the replace but a little better imo. That said, if we can do it with rollup that would be nice too. I dont have experience with it.

@thescientist13
Copy link
Member Author

thescientist13 commented Nov 10, 2020 •
edited
Loading

Fair point, but unfortunately there's no other way to render web components without a browser, so puppeteer is kind of our only option there. But say for local development where serialization doesn't occur, JSDOM seems like a lot of overhead for achieving what we want, which is effectively just a more ergonomic alternative to a string.replace(/* */), times all the instances we may need to invoke it.

Being able to quickly assemble bits of HTML on the fly might not be JSDOM's strong suit is all, and so I guess in my mind if all the problem calls for is just a tool for concatenating some string together in a programmatic way, a parser seems a little more specialized for the task. Certainly open to seeing alternatives in action, just that JSDOM seemed a bit heavy handed when just exploring the idea off the top of my head. (and we did already remove it from our flow once before, so I think there's a case to keep it out of our flow unless absolutely necessary).

@thescientist13 thescientist13 mentioned this issue Nov 12, 2020
Merged
12 tasks
@thescientist13 thescientist13 added CLI bug Something isn't working and removed enhancement Improve something existing (e.g. no docs, new APIs, etc) labels Nov 16, 2020
@thescientist13 thescientist13 added the good first issue Good for newcomers label Dec 4, 2020
@thescientist13 thescientist13 self-assigned this Feb 13, 2021
@thescientist13 thescientist13 mentioned this issue Feb 13, 2021
Merged
@thescientist13 thescientist13 linked a pull request Feb 13, 2021 that will close this issue
Bug/issue 431 template <head> tags merge order #475
Merged
@thescientist13 thescientist13 mentioned this issue Feb 25, 2021
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thescientist13 thescientist13

Labels
alpha.7 bug Something isn't working CLI good first issue Good for newcomers v0.10.0
Projects
None yet
Milestone
No Bundle Feature Parity
Development

Successfully merging a pull request may close this issue.

Bug/issue 431 template <head> tags merge order ProjectEvergreen/greenwood
2 participants
@thescientist13 @hutchgrant