Enhancement/issue 434 expand script style link tag production bundling #472

thescientist13 · 2021-02-04T02:09:58Z

Related Issue

resolves #434

I gotchu fam

Summary of Changes

Refactored rollup.config.js to align on a single html parsing library
Now the below type of inline <script> tag usage will get properly bundled by Rollup and inlined back into the HTML
```
<script type="module">
  import '@bare/specifier';
<script>
```
Added test cases and fixed a rollup <script> tag matching bug - (also seen in https://github.com/ProjectEvergreen/greenwood/pull/475/files#diff)
Updated docs with notes on a recommendation to use file based style and script tag usage

TODO

I think JavaScript handling breaks on multiple usages of <script src="..."> in the same document... (looks like this is due to a bug I saw happening inhttps://github.com/Bug/issue 431 template <head> tags merge order #475/files#diff-57634df7e2b1ba3ea0031d8adc1e73230ca3c93bbecfdea7b44d00c7e100ec8b as well)
Tests (think I will add this the import case added in issue 429 javascript module imports #465 once it gets merged )
~~Would like handle similar case for inline CSS, e.g.~~ - can't do this easily, just yet. how to avoid Puppeteer added inline styles?
```
<style>
  @import '../some.css'
  .my {
     .nested-styles {
      }
  }
</style>
```
Documentation
Clean up intermediate emitted (scratch) files
pick Buffer or crypto

Nice To Have

Would like to also support / verify this, but will likely just push this off to another issue for the 1.0 release

  <script type="module" src="@bare/specifier"></script>
  <link rel="stylesheeet" href="@bare/specifier"/>

avoid commented out code - this seems to happen already?

Summary / Thoughts / Takeaways

So for where this stands as of now, the general (to be) documented recommendation will be to use a file based entry point for all your scripts and styles, for anything anything doing any "heavy lifting" as this will afford the most opportunity for full and consistent bundling and optimization (at this time).

The mean reason is that since inline <script> and <style> don't initiate any fetch / network requests, the ability for the Greenwood server to intercept these and run tranforms and optimizatoin's is limited and so may lead to different outcomes between build and develop commands. This is something we could try and support, but I think can come at a later time. For now, we can at least do node_modules directly, which is pretty great.

Here are some examples of what is / isn't supported.

<!-- this will work -->
<script type="module" src="/your/scripts/main.js"></script>
<script type="module" src="/node_modules/some-package/index.js"></script>
<script type="module">
  import 'some-lib'; // bare import from node_modules

  // inline JavaScript works just fine!
  window.onload = () => {
     const route = window.location.pathname.split('/')[1];
        
     document.getElementsByTagName('app-shelf')[0].setAttribute('page', route);
  }
</script>

<style>
  p {
    color: red;
  }
</style>

<link rel="stylesheet" href="/your/styles/main.css"></link>
<link rel="stylesheet" href="/node_modules/some-package/styles.css"></link>


<!-- this will NOT work -->
<style>
  /* @import only works in a stylesheet? */
  @import url('./some.css')
  
  /* cant transpiling, like when using PostCSS for nested styles */
  p {
    & span {
      color: red;
    }
 }
</style>

thescientist13 · 2021-02-06T15:41:31Z

Had some more time to play around with this and the CSS side of things in particular, and I think there may be some caveats to keep in mind with supporting these two use cases in particular.

<script type="module">
  import '@some/code';
</script>

<style>
  div {
     a {
       color: blue;  /* notice nested selectors */
     }
  }
</style>

because none of this will get handled by Koa, since it won't trigger a network request, I think it might be counter intuitive to run Babel / PostCSS on this code in Rollup since this would not be something users would be experiencing during development (for instance, nested selectors would not work), and so I think this introduces conflicting outcomes between development and production, which I think would not be a good workflow to enable.

That said, I think we can still apply basic optimizations like minification (terser / cssnano) for free on these that we could put into the optimize functions our of standard CSS / JS resources like I started to do in #465.

The more I've been thinking about this, the more I don't think we should be calling PostCSS (or Babel) directly in rollup.config.js either, as I don't want to couple these tools, since there is more options than these tools for optimizing code and so makes a much better case for plugins to take up this task (e.g. optimize). I actually plan on doing some refactoring as part of #426 by making both PostCSS and Babel standalone plugins, and refactor our rollup.config.js accordingly. We can move cssnano to the optimize function of our standard CSS plugin, and thus we can actually push all the PostCSS plugins out of the CLI too!

That said, I could definitely see us supporting these use cases fairly easily still...

<script type="module" src="@evergreen-wc/eve-container">
<script type="module" src="@package/dist/index.js">

<link rel="stylesheet" href="@bootstrap/bootstrap">
<link rel="stylesheet" href="@bootstrap/bootstrap/dist/bootstrap.css">

But I would like to merge #465 first for it's refactoring, and then wrap up #426 , since it would introduce even more refactoring.

…ogic accordingly

thescientist13 · 2021-02-24T01:31:48Z

So in the case here, where we can potentially have different bundling / optimizations in build and develop based on that develop is mostly driven by network requests, like import, which uses fetch under the hood, I think the best place to leave them at is on equal footing.

We can certainly make it work such that plugin-resource-html can scan for inline <style> or <style> tags and reach out the user and other internal plugins to process the inline code content. Once we can do that, I think we can make sure to promote / ensure that Rollup will do the same. Added as a note for us to track in #418, though not sure if it's something we should aim for 1.0.0, or post 1.0.0.

#472) * remove duplicate html parsing lib and refactor script and link emit logic accordingly * refactor rollup script and link tag updates to new html parser * handling bundling of inline script tags * code cleanup * wip * fixed bug with the implementation * restore optimization * add specs for script style link tag usage * add test for inline script with node_modules * use Buffer instead of cyrpto * remove comment * fix bug with incorrect script tag updating in rollup * fix bug with incorrect script tag updating in rollup * clean up scratch files and assert number of output bundles * support node modules via link and script tags * add specs for node modules link and style use cases * naming refactor * docs * small refactor

thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) CLI labels Feb 4, 2021

thescientist13 requested a review from hutchgrant February 4, 2021 02:09

thescientist13 self-assigned this Feb 6, 2021

thescientist13 added the documentation Greenwood specific docs label Feb 6, 2021

thescientist13 mentioned this pull request Feb 9, 2021

Enhancement/issue 426 restore postcss as plugins #473

Merged

5 tasks

thescientist13 linked an issue Feb 14, 2021 that may be closed by this pull request

handle bare imports in HTML <script> tags when running build command #434

Closed

5 tasks

thescientist13 force-pushed the enhancement/issue-434-expand-script-style-link-tag-production-bundling branch from 945e642 to 3dfd106 Compare February 18, 2021 20:17

thescientist13 added the question Further information is requested label Feb 21, 2021

thescientist13 added 17 commits February 21, 2021 11:29

remove duplicate html parsing lib and refactor script and link emit l…

e8a3ff8

…ogic accordingly

refactor rollup script and link tag updates to new html parser

f76488e

handling bundling of inline script tags

a96320b

code cleanup

4fc6541

wip

136495f

fixed bug with the implementation

0be381b

restore optimization

b8c3a0e

add specs for script style link tag usage

10ddb6e

add test for inline script with node_modules

1410014

use Buffer instead of cyrpto

40acde3

remove comment

32dbe14

fix bug with incorrect script tag updating in rollup

2607267

fix bug with incorrect script tag updating in rollup

48e326d

clean up scratch files and assert number of output bundles

cb69f61

support node modules via link and script tags

712ea64

add specs for node modules link and style use cases

44883f8

naming refactor

ef24291

thescientist13 force-pushed the enhancement/issue-434-expand-script-style-link-tag-production-bundling branch from 5793a23 to ef24291 Compare February 21, 2021 18:54

thescientist13 added the website Tasks related to the projects website / documentation label Feb 21, 2021

docs

15d242e

thescientist13 marked this pull request as ready for review February 21, 2021 19:38

thescientist13 removed their assignment Feb 21, 2021

small refactor

5fb39a6

thescientist13 mentioned this pull request Feb 24, 2021

Roadmap To 1.0 And Beyond #418

Closed

53 tasks

thescientist13 removed the question Further information is requested label Feb 24, 2021

thescientist13 merged commit 4bde3f3 into release/0.10.0 Feb 27, 2021

thescientist13 deleted the enhancement/issue-434-expand-script-style-link-tag-production-bundling branch February 27, 2021 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/issue 434 expand script style link tag production bundling #472

Enhancement/issue 434 expand script style link tag production bundling #472

thescientist13 commented Feb 4, 2021 •

edited

thescientist13 commented Feb 6, 2021 •

edited

thescientist13 commented Feb 24, 2021 •

edited

Enhancement/issue 434 expand script style link tag production bundling #472

Enhancement/issue 434 expand script style link tag production bundling #472

Conversation

thescientist13 commented Feb 4, 2021 • edited

Related Issue

Summary of Changes

TODO

Summary / Thoughts / Takeaways

thescientist13 commented Feb 6, 2021 • edited

thescientist13 commented Feb 24, 2021 • edited

thescientist13 commented Feb 4, 2021 •

edited

thescientist13 commented Feb 6, 2021 •

edited

thescientist13 commented Feb 24, 2021 •

edited