Bundle scroll-time polyfill into library

[Smarthard]

resolves #572

I was able to figure out why tests were failing. Karma just wasn't serving new library's assets directory. Sorry to kept you waiting.

I added polyfill as is. Not sure if we actually need to provide a license notice or something like that there. But as soon as original repository will provide a proper npm package we could be able to remove it from here and work with it as a plain dependency.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c0ea0d9) to head (e0bb3e5).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #582   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          494       494           
  Branches        47        47           
=========================================
  Hits           494       494           

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

[MurhafSousli]

@Smarthard Thanks for the PR! could you double check why local coverage is broken

[Smarthard]

@MurhafSousli not sure. Probably your CI workflow missing write permission. At least that's what I found at marocchino/sticky-pull-request-comment readme. Here is the related issue: marocchino/sticky-pull-request-comment#298

[MurhafSousli]

@Smarthard I opened a PR to test the build, it has no issue #585

Weird, perhaps it couldn't comment on the PR cause it is opened by you?

[Smarthard]

I created a test branch with changes similar to yours in #585 and opened PRs #586 here and Smarthard#1 in my fork to test if it really a permission's issue

UPD: force-pushed to trigger CI build in fork repo

[Smarthard]

@MurhafSousli Yeah looks like you were right. My changes passed workflow in my repo and failed in yours. I think you need something like this to solve this issues with permissions.

diff --git a/.github/workflows/integrate.yml b/.github/workflows/integrate.yml
index 7ffcc0e..2729b37 100644
--- a/.github/workflows/integrate.yml
+++ b/.github/workflows/integrate.yml
@@ -8,6 +8,10 @@ on:
 
 jobs:
   build:
+    # Allow forks to be annotated with code coverage properly
+    permissions:
+      pull-requests: write
+
     # Machine environment:
     # We specify the Node.js version manually below, and use versioned Chrome from Puppeteer.
     runs-on: ubuntu-latest

[MurhafSousli]

I think you should try that change within your PR

[MurhafSousli]

I fixed the CI build issue, I got a question, the script is copied to assets directory of the plugin. but how would be copied to the user's app assets directory? would that happen automatically?

[Smarthard]

I fixed the CI build issue, I got a question, the script is copied to assets directory of the plugin. but how would be copied to the user's app assets directory? would that happen automatically?

By including it to angular.json build options as any other assets I suppose. Yeah it's a breaking change

UPD
something like this should work (I removed unnecessary lines):

{
    "projects": {
        "app": {
            "architect": {
                "build": {
                    "builder": "@angular-devkit/build-angular:browser",
                    "options": {
                        "assets": [
                            {
                                "glob": "**/*",
                                "input": "node_modules/ngx-scrollbar/assets/",
                                "output": "assets"
                            }
                        ],
                    }
                }
            }
        }
    }
}

[MurhafSousli]

In this case, I want to keep loading the script from the github link as the default option, without breaking changes and the only change in this PR would be the shipping of the script with the package.

We can add to the documentation the changes needed to load it manually from the assets directory.

[Smarthard]

@MurhafSousli Hello!

I updated PR as you required. All changes were squashed into single commit and force-pushed to make it clearer.

[leblancmeneses]

I have a related issue. I'm using this scrollbar in this chrome extension.
https://chromewebstore.google.com/detail/gihejcnfnbdiohnjepkkdhilfedjcahm/preview?hl=en&authuser=0&pli=1
https://twitter.com/leblancmeneses/status/1794105480043737531

It is being rejected due to this component not being fully contained.

Violation reference ID: Blue Argon

Technical Requirements - Additional Requirements for Manifest V3:

Violation: Including remotely hosted code in a Manifest V3 item.
Violating Content:
Code snippet: pages/sidepanel/browser/chunk-LQBSPELQ.js: https://flackr.github.io/scroll-timeline/dist/scroll-timeline.js

I've just come across an interesting point: you provide an injection token, NG_SCROLLBAR_POLYFILL, with provideScrollbarPolyfill. Unfortunately, I think they use static code analysis instead of actual runtime checking.

Remote code is any JS or Wasm that is not included in the extension's package. This includes references to external > files in <script> tags, modules pointing to external files, and strings evaluated through eval()

At runtime, I don't see this loading in the Chrome extension inspect window. So, if it is static code analysis, maybe change NG_SCROLLBAR_POLYFILL to supply a function that can be tree-shaken by consumers who do not need this polyfill. I wouldn't include it in a Chrome extension and for those that do they can override with their own function to load from a local location if they need that.

for now just so I can move forward I'm going using https://www.npmjs.com/package/patch-package to remove the reference.

[MurhafSousli]

@leblancmeneses I don't get it, in Chrome the polypill script does not even need to load, and the URL is stored as a string value. could you open an issue and describe what is not working?

[MurhafSousli]

@Smarthard Sorry for the late response. I added the documentation here https://github.com/MurhafSousli/ngx-scrollbar/wiki/Scroll-timeline-polyfill

[Smarthard]

@MurhafSousli no worries! Thanks! I just tested and it's working great.

One good thing to point out for anyone who will use it that way. I needed to additionally modify my CSP rules for style-src like this: style-src 'unsafe-inline' blob:. That's because polyfill trying to dynamically assign styles via createObjectURL.

For future references I'd like to left an example how I handled these changes for my project: Smarthard/shikicinema@c5ae774

[Smarthard]

@leblancmeneses frankly, from my experience moderators from Chrome Web Store often falsy-positive report issues on items to their developers. My recommendation is to reply to CWS from email message you receive (look closely for reply contacts). You could describe there your concerns about false-positive alert and provide them references from your code and/or references from the software your are using.

[MurhafSousli]

@Smarthard Is #590 or #594 related?

It would be beneficial if you could add a how-to make it work with CSP at the end at this documentation https://github.com/MurhafSousli/ngx-scrollbar/wiki/Scroll-timeline-polyfill

[Smarthard]

Is #590 or #594 related?

I think I've seen those kind of errors in my console as well, but after CSP correction there are no errors.

It would be beneficial if you could add a how-to make it work with CSP at the end at this documentation

For common SSR/CSR angular web apps this should be sufficient:

<!-- index.html -->
<meta http-equiv="Content-Security-Policy" content="style-src 'unsafe-inline' blob:">

For web extensions you would need to modify manifest.json of your extension, changes to index.html are not necessary in this case:

"content_security_policy": "style-src 'unsafe-inline' blob:"

[leblancmeneses]

@MurhafSousli

After removing the string with patch-package, my Chrome extension was approved.
https://chromewebstore.google.com/detail/tabsift/gihejcnfnbdiohnjepkkdhilfedjcahm?hl=en&authuser=0

I think the best solution here would be to bump the major version and change NG_SCROLLBAR_POLYFILL

export type PolyFillLoader = () => Promise<void>; // the function itself would contain the URL string.
export const NG_SCROLLBAR_POLYFILL = new InjectionToken<PolyFillLoader>('NG_SCROLLBAR_POLYFILL');

and figure out a way to make it tree-shakable for consumers wanting to override the current default behavior.

Thanks for maintaining this!

@Smarthard, This violation is pretty clear: 'modules pointing to external files'. I'm curious, though, if yours is approved with the string in your bundles.

[MurhafSousli]

So if any variable contains a string value of an external URL would be rejected even if it is never used at runtime! that's weird