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

Offer a Node API [15] #1532

Closed
donaldpipowitch opened this issue Nov 26, 2018 · 19 comments

Comments

Projects
None yet
5 participants
@donaldpipowitch
Copy link

commented Nov 26, 2018

  1. Propose a new feature/change:

    Write a short description of your proposal with (if applicable)
    some examples of the expected behaviour.

As far as I can tell the hint package only exports an Engine class which doesn't offer enough functionality to use hint with a regular Node API, because a lot of the common functionality (e.g. loading configs) is hidden in the CLI:

process.exitCode = await cli.execute(process.argv);

It'd be nice, if there was an easy to use Node API with a similar "interface"/config as the CLI.


One of my use cases: I'm still not entirely sure how the integrate hint to check my whole website. I don't just want to call the CLI and give one or two static URLs. I want to create some custom crawl logic for example and want to call hint when I need it.

@molant

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

You can import any file we are using internally for your own use. That's what we do in extension-vscode to load some specific resources and change the detected configuration (we force a parser and change the connector IIRC).

The crawler request has been asked a couple times (last week by @mgifford in this tweet). We could look into creating something that will be supported officially depending on the requirements (e.g.: pass a .txt with links, or a sitemap, or something similar).

In any case, @antross and I have talked in the past about this and you are right it's not easy to use it unless you know the code base well. What do you think of the following tasks for us:

  • Create new exports that simplify the paths of the most common used files for better consumption. Maybe something like import hintAPI from 'hint/api' that exposes Engine, load configurations, etc. (or something similar)
  • Create a guide on how to use hint's API with an example (maybe sitemap support for a crawler?)
@donaldpipowitch

This comment has been minimized.

Copy link
Author

commented Nov 27, 2018

Thank you for the feedback!

Create new exports that simplify the paths of the most common used files for better consumption. Maybe something like import hintAPI from 'hint/api' that exposes Engine, load configurations, etc. (or something similar)

Personally I'd prefer top use only top-level exports. I actually haven' tried something like import * as config from 'hint/dist/src/lib/config';, because I treat deep imports in projects which have a build step as private by default (- at least, if it isn't documented somewhere explicitly -), because they are probably bundled with tools like Rollup/Webpack in the future. (I actually thought that this was already the case and that a deep require wouldn't work. 🙈)

Create a guide on how to use hint's API with an example

😍

maybe sitemap support for a crawler?

The crawler feature is something I thought about for a while as well. I'm still not really sure how to solve this or if it should be solved in hint at all. Some alternative which came to my mind was something like this: Instead of including crawling logic into hint we could start a hint server or maybe a hinter "container/shell/runner" (similar to how Cypress embeds a page) which offers an API to start and stop hinting.

Something like this (Pseudo API):

// hint-runner could be a small wrapper around puppeteer which opens a
// predefined "container" (which embeds your real page in an iframe or something like that)
// and maybe has some predefined element handlers
const { launch, startRecording, stopRecording,  finish } = require('hint-runner');

launch().then(async (page) => {
  // maybe `startRecording` was implicitly called by default on startup

  // like puppeteers `page`, but scoped to the iframe
  await page.goto('example.com');

  // now visit some others site with the regular puppeteer API - everything is recorded

  // maybe we want to swich to a different site now (e.g. a login page maintained by a 3rd party
  // service like Auth0 which shouldn't be hinted)
  await stopRecording();
  // do login logic...

  // ...and start recording again
  await startRecording();

  // now visit some sites which need authentication

  // also calls `stopRecording` and returns _all_ the coverage information
  const results = await finish();

  // do something with results (e.g. throw on errors or whatever)
});

So in the most simplest case where you only need to crawl a couple of sites and collect the coverage for all sites in one go, but don't need to handle start and stop it could look like this:

const { launch, finish } = require('hint-runner');

launch().then(async (page) => {
  await page.goto('example.com/site-1');
  await page.goto('example.com/site-2');
  await page.goto('example.com/site-3');

  const results = await finish();
  // do something with results...
});
@renestalder

This comment has been minimized.

Copy link

commented Feb 13, 2019

Looking forward to this. It can especially help to test pages that need certain steps e.g. login masks before reaching the actual page that should be tested. Or for accessibility, testing different states of a page.

@molant molant referenced this issue Feb 16, 2019

Closed

Cdp and jsdom using a copy of the DOM #1910

4 of 4 tasks complete

@molant molant added this to the 1903-1 milestone Feb 25, 2019

@antross antross modified the milestones: 1903-1, 1903-2 Mar 8, 2019

@sarvaje sarvaje self-assigned this Mar 11, 2019

@molant molant referenced this issue Mar 12, 2019

Closed

Breaking: Add API #2052

2 of 4 tasks complete
@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

I have been taking a look to all the hint references/dependencies and these are the most used (there is others but these are the most used)

I'm not sure if exposing the utils is useful for what @donaldpipowitch or @renestalder need, but definitely it is useful if we want to use it in the hint package, instead of using the path to what we need.

In that case, I think we should expose all the utils, even if it isn't in the list below, because it doesn't mean it cannot be use in a future hint or formatter.

Also, I agree with @molant in that formatters is a must-have.

I also find out that we are using the path to some other packages, not just hint, for example @hint/utils-tests-helpers/dist/src/hint-runner. In these cases, I think we should also expose what we need in that package, so the user just have to import, in this case, @hint/utils-tests-helpers and doesn't need to know the internal structure of the package.

dependency Resource Type
@hint/utils-connector-tools/dist/src/normalize-headers connector code
@hint/utils-connector-tools/dist/src/requester connector, hint code
@hint/utils-debugging-protocol-common/dist/src/debugging-protocol-connector connector code
@hint/utils-debugging-protocol-common/dist/src/launcher connector code
@hint/utils-tests-helpers/dist/src/hint-runner create, hint template, test
@hint/utils-tests-helpers/dist/src/hint-test-type create, hint template, test
hint/dist/src/lib/config extension, util code
hint/dist/src/lib/engine (this one can be replaced by hint) connector, create, parser, util code, template
hint/dist/src/lib/enums/category create, extension, formatter, hint code, doc, template, test
hint/dist/src/lib/enums/hint-scope create, hint code, template
hint/dist/src/lib/enums/resource-type create code
hint/dist/src/lib/hint-context create, hint code, doc, template
hint/dist/src/lib/types connector, create, extension, formatter, hint, parser, util code, doc, template, tests
hint/dist/src/lib/utils/app-insights create code
hint/dist/src/lib/utils/async-wrapper hint code, test
hint/dist/src/lib/utils/caniuse hint code
hint/dist/src/lib/utils/content-type connector, extension, hint, parser, util code
hint/dist/src/lib/utils/debug connector, create, formatter, hint, parser, util code, template
hint/dist/src/lib/utils/dom/create-html-document connector, extension, parser, util code
hint/dist/src/lib/utils/dom/get-element-by-url connector, extension, util code
hint/dist/src/lib/utils/dom/traverse connector, extension, util code
hint/dist/src/lib/utils/fs/cwd connector, create code, test
hint/dist/src/lib/utils/fs/file-extension hint code
hint/dist/src/lib/utils/fs/is-file connector code
hint/dist/src/lib/utils/fs/load-json-file formatter, hint, parser code, test
hint/dist/src/lib/utils/fs/read-file connector, create, extension, hint, parser code, test
hint/dist/src/lib/utils/fs/read-file-async connector, create, hint code, test
hint/dist/src/lib/utils/fs/write-file-async connector, create, hint, parser code
hint/dist/src/lib/utils/hint-helpers create, hint doc, template, test
hint/dist/src/lib/utils/json-parser parser code
hint/dist/src/lib/utils/logging connector, create, formatter, hint, parser code
hint/dist/src/lib/utils/misc/cut-string formatter, hint, util code, test
hint/dist/src/lib/utils/misc/delay connector, hint, util code, test
hint/dist/src/lib/utils/misc/generate-html-page connector, create, hint template, test
hint/dist/src/lib/utils/misc/normalize-string create, hint, parser code
hint/dist/src/lib/utils/misc/normalize-string-by-delimeter create code
hint/dist/src/lib/utils/misc/pretty-print-array hint code, test
hint/dist/src/lib/utils/misc/to-camel-case create code
hint/dist/src/lib/utils/misc/to-lowercase-keys util code
hint/dist/src/lib/utils/misc/to-pascal-case create code
hint/dist/src/lib/utils/network/as-path-string connector, parser code, test
hint/dist/src/lib/utils/network/as-uri connector, parser, util code, test
hint/dist/src/lib/utils/network/is-data-uri hint code
hint/dist/src/lib/utils/network/is-html-document connector, util code
hint/dist/src/lib/utils/network/is-http hint, parser code
hint/dist/src/lib/utils/network/is-https hint, parser code
hint/dist/src/lib/utils/network/is-local-file hint code
hint/dist/src/lib/utils/network/is-regular-protocol hint code
hint/dist/src/lib/utils/network/normalized-header-value hint, utils code
hint/dist/src/lib/utils/network/request-async extension, hint, parser config, code
hint/dist/src/lib/utils/network/request-json-async hint code
hint/dist/src/lib/utils/npm create code
hint/dist/src/lib/utils/packages/is-official create code
hint/dist/src/lib/utils/packages/load-hint-package create code
hint/dist/src/lib/utils/packages/load-package hint, parser code, test
hint/dist/src/lib/utils/resource-loader create, extension, util code
hint/dist/src/lib/utils/schema-validator parser code
@molant

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@sarvaje I've updated your table to have the references sorted alphabetically. I think that will make it easier to see how we group things.

@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@sarvaje I've updated your table to have the references sorted alphabetically. I think that will make it easier to see how we group things.

Ok, it was sorted from the most used to the less, but this is ok too hehehe.

@molant

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

it was sorted from the most used to the less, but this is ok too hehehe.

If we plan on shipping all utils together I think this is easier to see where things are.

I've added a new column type. The idea is to fill it with information on how it is being used. I'm seeing quite a few that are only used in the tests for example. For those maybe we shouldn't expose them directly but rather do an abstraction. Maybe we should add another column saying what type of resource uses it? Extension, hint, parser, all, etc.
Can you please fill the information?

@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Can you please fill the information?

sure

@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

I thing that for some packages like @hint/utils-debugging-protocol-common we should make them expose their things. for example, @hint/utils-debugging-protocol-common/dist/src/debugging-protocol-connector should expose Connector and Launcher so instead of having:

import { Launcher } from '@hint/utils-debugging-protocol-common/dist/src/launcher';

we would have:

import { Launcher } from '@hint/utils-debugging-protocol-common';

I thinks we should do this with all the @hint/utils-*

@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@molant the table is complete

@molant

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

I thing that for some packages like...

That sounds right. Can you come up with a proposal for all the other changes? What should we expose? Should we create a new package, etc.?

@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Can you come up with a proposal for all the other changes? What should we expose? Should we create a new package, etc.?

ok!

@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Here is my proposal for this task:

API:

I don't have a strong opinion if we should have a new package for this or if we should use hint.

The API will have an object webhint.

This object will have a method initialize which will accept as a parameter the configuration (json format, and it will be optional, if no configuration is passed, we can use configuration-web-recommended by default). and it will return an object with a method analize.
This will allow us to scan more than one URL with the same configuration without having to initialize the configuration in each scan.

analize will accept as a parameter the url to analize. This method will create a new Engine instance everytime it is call, so we can call analize in parallel without problems. analize will return an object with the result. This object can be used as parameter in a formatter to print the result.

Also, the webhint should expose the formatters (already initialized)

This is how it will look in code:

import { webhint } from 'hint-api'; // or import { webhint } from 'hint';

const analyzer = webhint.initialize(jsonConfiguration);
const result = analyzer.analyze('urlToAnalyze');

webhint.formatters.summary.format(result);

Current Util packages:

  • @hint/utils-connector-tools:

    • Rename @hint/utils-connector-tools to @hint/utils-connector.
    • Expose:
      • normalizeHeaders
      • requester
      • redirects. I think we are not using it outside this package, but if it is here, we should expose it
    • delete or expose:
      resolver. We are not using it anywhere, but we have code that does the same in some places. If we decide to keep it, we need to use it in those places.
  • @hint/utils-debugging-protocol-common:

    • Expose:
      • Connector
      • Launcher
      • RequestResponse. As redirects, we are not using this outside this package, but we should expose it anyways.
  • @hint/utils-tests-helpers:

    • Expose:
      • testHint
      • testLocalHint
    • We also should expose the types: Report, HintTest and HintLocalTest

New Util package(s):

I see two options here:

  1. Create a new package @hint/utils where we will move the folder packages/hint/src/lib/utils, and expose all the utils.
  2. Create a new package for each folder:
    • @hint/utils for all the utils in the utils root folder.
    • @hint/utils-dom for all the utils in the dom folder.
    • @hint/utils-fs for all the utils in the fs folder.

In any case, we should move to the new packages the types that were created fo them, e.g. json-parser

Types and enums:

Should we move types and enums to another package?
If not, hint should expose all of them.

Formatters:

Formatters are already exposed, so I think we don't need to touch anything here.

@molant

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

API

This looks like a good starting point.

In terms of the api I think you forgot to make it async:

import { webhint } from 'hint-api'; // or import { webhint } from 'hint';

const analyzer = webhint.initialize(jsonConfiguration);
const result = await analizer.analyze('urlToAnalize');

webhint.formatters.summary.format(result);

From what I'm reading webhint is the one in charge of loading the resources, correct? We should document the different errors we can get and see how the CLI will use them to inform of the problem to the user. Some that come to my mind:

  • JSON object is not valid --> Who is going to do the schema validation? The CLI? Do we do another type of validation in webhint as well?
  • Resources not found or incompatible versions found
  • Configuration for a hint is not valid
  • ?

New Util package(s)

I'm leaning towards having a separate utils package. That should break some of the weird circular depedendencies we have. I think that having just one is more than enough. It should be pretty stable and I don't see us making any breaking change in the near future (probably more like adding but that will be minors so it should be fine).

I think I'm ok moving everything under utils (or almost everything). hint is going to depend heavely on this and the create packages will just bundle the necessary parts so size shouldn't increase.

@antross what do you think?

@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

In terms of the api I think you forgot to make it async:

Yep, it should be async.

Who is going to do the schema validation?

initialize should do all the malidation you mention, IIRC, the idea is reuse the API in the CLI too, right? This way I think we will fix also #1595

@antross

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Looking good. Let's make sure we call the API analyze as the current spelling has a whole different meaning...

I like having a single, separate utils package for the general-purpose utilities, though I could see having a few additional ones for utilities geared at specific consumers (e.g. @hint/utils-hint, @hint/utils-parser, etc.) similar to @hint/utils-connector-tools and @hint/utils-test-helpers today.

In general I like splitting utilities based on the expected audience instead of what they provide. Certain utilities may also make sense to move to an existing utility package (e.g. utils/dom/traverse.ts could move to @hint/utils-connector-tools).

This may also be a good time to eliminate a few of our utilities in favor of 3rd-party equivalents, e.g. utils/misc/to-camel-case.ts could easily be replaced by lodash which we already use elsewhere in the project.

I agree I don't want a separate types package as that would make things unnecessarily cumbersome. Types that hint creates should be exposed in hint and types that are specific to a util should be exposed in the appropriate utils package.

@sarvaje

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Looking good. Let's make sure we call the API analyze as the current spelling has a whole different meaning...

Updated hahaha.

@molant @antross do you want me to start working on this or do we need to discuss anything else?

@molant

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Can you please look into #2068 before jumping into this?

@molant molant modified the milestones: 1903-2, 1904-1 Mar 22, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Mar 26, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Mar 28, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Mar 29, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Mar 29, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Mar 29, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 2, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 2, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 2, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 2, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 3, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 3, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 3, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 3, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 4, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 4, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 4, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 8, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 8, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 8, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 9, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 9, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 9, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Apr 10, 2019

@molant molant closed this in d934aeb Apr 10, 2019

@donaldpipowitch

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

Thank you very much!

@molant molant changed the title Offer a Node API Offer a Node API [15] Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.