Skip to content

Conversation

@codycraven
Copy link

The old version of the script was limited to only parsing for the og namespace. This revision adds support for parsing for any namespaces desired, which unfortunately breaks the expected response for prior implementations however it is a necessary improvement.

As a note the old implementation failed to work on sites following current Open Graph standards from http://ogp.me due to modifications in the namespace prefix declaration. This revision forgoes the forcing of prefix detection from jsdom since the Open Graph standard has changed at least three times that I am aware of.

I updated the package.json version a major revision, since updating the module would break any existing implementations expecting results such as { title: 'Title value' } which would now be { 'og:title': 'Title value' } due to the multiple namespace support.

The old version of the script was limited to only parsing for the og
namespace. This revision adds support for parsing for any namespaces
desired, which unfortunately breaks the expected response for prior
implementations however it is a necessary improvement.

As a note the old implementation failed to work on sites following
current Open Graph standards from http://ogp.me due to modifications
in the namespace prefix declaration. This revision forgoes the forcing
of prefix detection from jsdom since the Open Graph standard has changed
at least three times that I am aware of.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This namespace prefix declaration does not match current http://ogp.me/ specifications and as such prevented the script from working on Open Graph valid pages. I removed this requirement all together as I know of at least three different implementations the Open Graph protocol has declared.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whey don't keep this check and just add conditional for new namespace?

@elf-pavlik
Copy link

@SpeCT what do you think about those changes? at this moment this package doesn't work with most valid sites and http://ogp.me now recommends defining a prefix in a different way... i would look at just adding search for new prefix and then follback to 'og' if nothing found...

@elf-pavlik
Copy link

@codycraven for what usecase you want to support multiple namespace prefixes? once we get prefix for searching and then remove it from results it doesn't matter.... i would just consider

  1. look for old namespace to find prefix
  2. if 1 fails look for new namespace to find prefix (checking both possibilities of stating it, xmlns and prefix attributes)
  3. if 2 also fails fallback to 'og' prefix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants