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

wrapper for angular #482

Closed
Vwings opened this issue Aug 27, 2021 · 21 comments · Fixed by #526
Closed

wrapper for angular #482

Vwings opened this issue Aug 27, 2021 · 21 comments · Fixed by #526
Labels
enhancement New feature or request

Comments

@Vwings
Copy link
Contributor

Vwings commented Aug 27, 2021

Is there any plan to make a wrapper for angular?

@dvelasquez
Copy link
Owner

Hello @Vwings , thanks for opening an issue.

I've been waiting for someone to show interest in an Angular wrapper, and you are the first!

This is something I could use some help, last time I used Angular was en version 2, and I've never published an Angular component before.

Is this something you could be interested to contribute with?

@dvelasquez dvelasquez added the enhancement New feature or request label Aug 30, 2021
@Vwings
Copy link
Contributor Author

Vwings commented Sep 1, 2021

@dvelasquez Thanks for your reply! I like the idea of ​​this project, let me see if I can contribute some code.

@dvelasquez
Copy link
Owner

@Vwings Let me know if you need any help or explanation on how it works 👍

@Vwings
Copy link
Contributor Author

Vwings commented Sep 22, 2021

Hello @dvelasquez , I've finished the angular wrapper, the package's name is ngx-lighthouse-viewer.
Here is the forked project: https://github.com/Vwings/lighthouse-viewer/tree/feat/angular-wrapper

I also created a demo project in packages/ngx-lighthouse-viewer/projects/demo.

What else can I do before creating pull request?

@dvelasquez
Copy link
Owner

In the root folder there is an e2e set of tests to run and they are located at cypress/integration.

There you should create an angular_spec.ts that should pass the same tests as the others.

Every wrapper is using the same version of the report.json, that makes easier to reuse 99% of the assertions in every wrapper.

I'll be back from holidays next week and then I'll make a more extensive code review.

Thanks very much for your interest and contributions!

@Vwings
Copy link
Contributor Author

Vwings commented Sep 24, 2021

Hello, angular cypress test is added and passed.

I added an Angular Demo link to the index.html in the root folder to pass the cypress test, parcel need a reference to load angular demo assets.

The angular demo currently don't contain the content of the public page. I noticed that each wrapper has a separate index.html file, and the report is rendered in an iframe. I tried to render the report in a div, but mvp.css conflicts with lighthouse viewer's stylesheet.

Maybe we can change the structure of the demo to this:

  • Wrappers only provide report html (lh-demo.html, for angular it's index.html)
  • index.html exists only in the root directory
  • index.html imports all the report html from wrappers by iframe.

I would appreciate hearing your opinion on this.

@dvelasquez
Copy link
Owner

dvelasquez commented Sep 27, 2021

Hey @Vwings thanks for your contribution, and thanks for adding the cypress tests and the demo.

Regarding the iframe thing: This issue is not only a MVP.css problem, but also a Lighthouse issue. The original Lighthouse reporter is overriding primitive elements (links, buttons, etc) and not using any type of scoping. So this can cause issues when using it in any other project (I've had several problems with this myself). This last bit is not longer true in the latest versions of Lighthouse. I'm updating the dependencies this week so I would suggest to make a rebase tomorrow of your fork.

You can check the refactor progress here: GoogleChrome/lighthouse#12254

I've used the iframe as a workaround so I don't have to worry about this. I like your approach and I would love to do that change but in a separate Pull Request. For now you can open a PR with this project being a little different.

I have some questions regarding the publication of the package and the scaffolding of the project. Until now I've been using this type of folder structure:

- packages
-- lighthouse-viewer
--- dist/
--- src/
--- package.json

And every package.json has this attributes:

"name": "lighthouse-viewer",
  "description": "Package with the lighthouse-viewer files from the official repositories of Lighthouse",
  "version": "0.1.19",
  "private": false,
  "main": "./dist/lighthouse-viewer.umd.js",
  "module": "./dist/lighthouse-viewer.esm.js",
  "types": "./dist/types/index.d.ts",
  "files": [
    "/dist"
  ],
}

So, whenever we run npm publish, the whole folder is packaged as a .tar file and uploaded to NPM.

But in the Angular Wrapper I see that the scaffolding is quite different:

- ngx-lighthouse-viewer
-- package.json
-- dist/
--- ngx-angular-viewer/
--- demo/
-- projects/
--- ngx-angular-viewer/
---- src/
---- package.json

So my question here is which package is going to be published to NPM?

I think it should be ngx-lighthouse-viewer/projects/npx-lighthouse-viewer, but for this to work, we should have the dist folder inside this directory.

Also we need to add the fields for main, module and types to the package.json that will be published.

We could also publish the root ngx-lighthouse-viewer, but the dependencies could make this a little more complicated.

Let me know what you think.

@Vwings
Copy link
Contributor Author

Vwings commented Sep 27, 2021

Thanks for your reply!

There is a publish script in packages/ngx-lighthouse-viewer/package.json.

The script will build library project, copy README and LICENSE files to dist folder, and finally publish the folder packages/ngx-lighthouse-viewer/dist/ngx-lighthouse-viewer to npm repository.

The following properties will also be added to the package.json generated by angular cli.

{
  // ...
  "main": "bundles/ngx-lighthouse-viewer.umd.js",
  "module": "fesm2015/ngx-lighthouse-viewer.js",
  "es2015": "fesm2015/ngx-lighthouse-viewer.js",
  "esm2015": "esm2015/ngx-lighthouse-viewer.js",
  "fesm2015": "fesm2015/ngx-lighthouse-viewer.js",
  "typings": "ngx-lighthouse-viewer.d.ts",
}

@dvelasquez dvelasquez linked a pull request Sep 28, 2021 that will close this issue
@dvelasquez
Copy link
Owner

The PR has been merged. Thanks very much for your contribution.

Quick question: would you be interested helping to maintain this project?

@Vwings
Copy link
Contributor Author

Vwings commented Sep 29, 2021

The PR has been merged. Thanks very much for your contribution.

Quick question: would you be interested helping to maintain this project?

Of course, I am willing to help maintain this project.

@dvelasquez
Copy link
Owner

Great I will add you as a contributor ;)

@dvelasquez
Copy link
Owner

@Vwings quick question, do you have an example in another repository using this package?

@Vwings
Copy link
Contributor Author

Vwings commented Nov 15, 2021

@dvelasquez Hello, I published the package to my repository and used in a private repo. If you are looking for a demo in separate project, I can create one and publish to github.

I've changed some features since the last pull request:

  • Support exporting HTML as a standalone HTML file.
    Currently the "Save as HTML" function simply get root element's outerHTML and save it to a file.
getReportHtml() {
    if (this._topbar) {
      this._topbar.resetUIState();
    }
    return `<!doctype html><body>${this._dom.rootEl.outerHTML}`;
}

I'm not using this component inside an iframe and want to generate a standalone HTML file like lighthouse, so I changed this function. And I added a script in lighthouse-viewer to build package without copying lighthouse's code.

  • Support changing theme, and emit an event when theme changed inside lighthouse report. this function is also wrapped in angular component.
  • Support compling with angular 10
  • Other customized requirements

Many of these are custom changes and is not suitable to merge into this repo.

I've seen the publish issue, is there any other way to publish packages?

@dvelasquez
Copy link
Owner

Cool features! So a couple of thoughts:

  1. The lighthouse team is actively working on make the lighthouse report viewer available as a ESM standalone package. This will make ours lighthouse-viewer pointless (hopefully) very soon.
  2. I don't have the expertise on Angular and dependabot + angular-cli are not playing well (build broke if we only update one of the dependencies and not all of them).
  3. The publish issue is a problem that I've faced in other projects but I haven't found the time to sit down and fix it.

The changes you have made sound very interesting. I would like to take a look to them if it's possible.

Now I'm going to focus on stabilise the build, update to Lighthouse 9 and fix the publish issue.

I would love to keep this project as simple as possible and be able to add custom code on top of it too.

@Vwings
Copy link
Contributor Author

Vwings commented Nov 17, 2021

Glad you are interested in my changes, here is the branch https://github.com/Vwings/lighthouse-viewer/tree/feat/export-listener_1.0.2.

For publish issue, currently there are 3 pacakge.json files in pacakges/ngx-lighthouse-viewer:

  • pacakges/ngx-lighthouse-viewer/package.json: this is a private package.
  • packages/ngx-lighthouse-viewer/projects/ngx-lighthouse-viewer/package.json: this is a template for angular package, should not be published.
  • packages/ngx-lighthouse-viewer/dist/ngx-lighthouse-viewer/package.json: this is generated after building, and should be published.

I'm not sure if the publish action will only recognize the third package.json. The angular wrapper is currently using a script to publish packages(npm run publish) , which is inconsistent with other wrappers.

If lerna publish can recognize the second and third packages, maybe I can modify the second package to private to make lerna only recognize the third package, then we can use lerna publish for all packages.

@dvelasquez
Copy link
Owner

dvelasquez commented Nov 17, 2021

Yes, we can do that @Vwings

Now we are using a wildcard for the package folder, but since the amount of packages is limited, we can specify directly, making this more explicit and with a more granular control.

The change would be from something like this:

{
  "command": {
    "publish": {
      "ignoreChanges": ["ignored-file", "*.md", "**/packages/**/demo/**"],
      "message": "chore(release): publish"
    }
  },
  "packages": [
    "packages/*"
  ],
  "version": "independent"
}

To something like this:

{
  "command": {
    "publish": {
      "ignoreChanges": ["ignored-file", "*.md", "**/packages/**/demo/**"],
      "message": "chore(release): publish"
    }
  },
  "packages": [
    "packages/lighthouse-viewer",
    "packages/react2-lighthouse-viewer",
    "packages/ngx-lighthouse-viewer/lib/etc..."
  ],
  "version": "independent"
}

I'm also wondering if it's possible to create an angular component/library without using the angular-cli.
The wrappers are simple and if we reduce the amount of libraries, cli and packages we use to make the component we can go around the issue with dependabot and angular ecosystem.

@Vwings
Copy link
Contributor Author

Vwings commented Nov 18, 2021

I'm also wondering if it's possible to create an angular component/library without using the angular-cli. The wrappers are simple and if we reduce the amount of libraries, cli and packages we use to make the component we can go around the issue with dependabot and angular ecosystem.

That's a good idea, I think I will try to use rollup or change the project's structure. I hope we can avoid adding special lerna configuration because of ngx-lighthouse-viewer.

@dvelasquez
Copy link
Owner

Hi @Vwings.

I've been having consistent issues with keeping the angular library up to date, and until today there is no fix or publication of the Angular package.

So, I've decided to remove support for Angular for the time being. I'm doing several changes that will require rewrite a lot of the parts, for example, ditch parcel1 for vite and possibly migrate from Lerna to NX.

Probably after the migration to NX, we will be able to add the Angular wrapper again. I don't see any issue if you want to take over the Angular wrapper and publish it yourself.

@dvelasquez
Copy link
Owner

I'm still not convinced of using NX to manage monorepos.

I will give it a try in the future to restore the Angular configuration and see how it goes. For now, I will close this PR.

Let me know if you are going to create and publish an Angular wrapper by yourself. If that is the case, I will not try to give it support from here.

@Vwings
Copy link
Contributor Author

Vwings commented Feb 7, 2022

Hi @dvelasquez
Angular has lots of own build configs, I think it's better to make the wrapper in a separated repository. I will publish the angular wrapper to npm.

@dvelasquez
Copy link
Owner

Great! Once is created don't forget to add it to the list of tools in the main Lighthouse repository for visibility: https://github.com/GoogleChrome/lighthouse#related-projects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants