jbhannah / amperize Goto Github PK

ugh

I'm gonna attach Dependabot to this repo, set to auto-merge security and patch updates. If anyone has any objections, put 'em here.

This is related to #97. When one image fails in the image-size library, the whole process is stoped because we return an error.

IMO we should swallow the returned error from image-size and add fake sizes to the <amp-img> tag to continue parsing the rest of the passed HTML.

Use image-size to detect image width and height if missing and if layout is fixed, responsive, or fixed-height.

AMP doesn't allow percentage values as width or height properties in iframe tags and will error. We should simply remove those properties when a % sign is found.

Hey @jbhannah,

could you please make a version bump after @kirrg001 merged changes, so we can use the new version in our deps?

Cheers, Aileen

• Replace img tag with amp-img, maintaining attributes
• Add layout="responsive" if layout attribute missing
• Configurable default layout attribute value
• Get image dimensions and set width and height attributes if missing and if layout is fixed, responsive, or fixed-height

var amp = new Amperize({
  img: {
    layout: "responsive"
  }
});

Why do you fail all amp content if one image has wrong src? please leavi it for config option- what to do with wrong src: throw error, ignore and put it as amp-img and try using alt, etc.

I fail with trying to amperize my html like this:

Currently we're using https or http to make requests to read the image buffer and then pass it to image-size. These don't follow redirects. The library got follows redirects and would simplify the request a lot.

With TryGhost/Ghost#8892 we switched to the got library for the very same task and are very happy with it. I suggest doing the same here.

Hi @jbhannah,

as we (Ghost) want to use Amperize to transform our HTML into valid AMP HTML (see here), I kindly request to make a release tag and continue with release cycles, so it's easier to use Amperize as a dependency and keep it updated.

It's a great tool with a lot of potential and I would love to see it becoming the AMP HTML transform lib on GitHub.

Cheers,

Aileen

Hi,

I installed the latest version of your package in a new project folder.

Created an index.js with the following content (based on your example in README.md)

const Amperize = require('amperize');

const html = '<img src="https://example.com/image.jpg" />';

try {
  const amperize = new Amperize();
  amperize.parse(html, function (error, result) {
      if (error) {
          // do something with error
          return new Error(err);
      }
      // do something with result
      console.log('amp html should be logged here...', result);
      return result;
  });
} catch (e) {
  console.log(e)
}

When I try to run via terminal with node index.js, it just returns the regular html string.

It's not converting to <amp-img> tag like it should.

Any ideas what could be happening?

Node v9.11.2
amperize: ^0.3.8
Mac OS

When an image without extension gets passed to the image-size library it will cause image-size to throw an error and therefore fail the rest of the conversion into AMP HTML.

IMO, we should look for the content-type as a fall back and manipulate the image name to add the extension.

We've run into an issue with amperize crashing our whole server process when an <img> tag has an href attribute with chars which are not properly encoded. Eg:

<img href="/images/Don’t%20Be%20Greedy.png" />

The problem is actually stemming from got which is not fully encoding the URL before passing on to the http module which then throws the exception. Unfortunately it seems that due to the way amperize is architected it's not possible to catch the exception in our app code because it's throwing from inside async/evented code.

Hi @jbhannah,

at the moment, we're only transforming a few HTML tags into AMP HTML. I would love to add more transformations (e. g. audio and video) in the near future. Nevertheless, some tags will still remain prohibited by AMP.

Here's the full list of restricted HTML tags by AMP: https://github.com/ampproject/amphtml/blob/master/spec/amp-html-format.md#html-tags

I would like to know, if you are planning to strip out prohibited HTML tags, so the output will be a 100% valid AMP HTML? Maybe with the help of the Sanitize-HTML module?

Would love to hear your opinion about that!

Cheers, Aileen

This issue is coming form TryGhost/Ghost/issues/10796

As we can see from here: https://github.com/ampproject/amphtml/blob/master/extensions/amp-iframe/validator-amp-iframe.protoascii

The iframe attribute needs proper sanitization as some attributes accept no value but their presence and some attrs allow values from [0, 1]

• scrolling: value as 0 or 1
• frameborder: value as 0 or 1
• allowtransparency: presence and no value
• allowfullscreen: presence and no value

Hey 👋

If a page has a large number of images and one image request times out, all images are not rendered.

https://github.com/jbhannah/amperize/blob/master/lib/amperize.js#L172

It would be great if amperize tries to fetch the image, if the request fails or times out, other images are still rendered.