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

fix issues link references and prototypes #1299

Merged
merged 1 commit into from
Jun 30, 2018

Conversation

Trott
Copy link
Contributor

@Trott Trott commented Jun 28, 2018

Marked version: 0.4.0

Markdown flavor: Markdown.pl

Description

Link with names that clashed with properties inherited from the
Object prototype (such as "constructor") were not expanding. This fixes
this issue.

Before this change, markdown of this form...:

Link: [constructor][].

[constructor]: https://example.org/

...resulted in HTML output of this form:

<p>Link: [constructor][].</p>

With this change, it now renders as expected:

<p>Link: <a href="https://example.org/">constructor</a>.</p>

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@UziTech
Copy link
Member

UziTech commented Jun 28, 2018

@joshbruce @styfle
this may need to be polyfilled if we intend on supporting ie8

Object.create Browser Compatibility

"markdown": "Link: [constructor][].\n\n[constructor]: https://example.org/",
"html": "<p>Link: <a href=\"https://example.org/\">constructor</a>.</p>",
"example": 10
},
Copy link
Member

Choose a reason for hiding this comment

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

Can you keep the example numbers in order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done. (I thought they were alphabetically ordered by section.)

Link with names that clashed with properties inherited from the
Object prototype (such as "constructor") were not expanding. This fixes
this issue.

Before this change, markdown of this form...:

    Link: [constructor][].

    [constructor]: https://example.org/

...resulted in HTML output of this form:

    <p>Link: [constructor][].</p>

With this change, it now renders as expected:

    <p>Link: <a href="https://example.org/">constructor</a>.</p>
@Trott
Copy link
Contributor Author

Trott commented Jun 28, 2018

this may need to be polyfilled if we intend on supporting ie8

The polyfill at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create does not support null as an argument. I suspect other polyfills may have the same shortcoming.

Would this be tolerable?

// IE8 does not have Object.create()
this.tokens.links = Object.create ? Object.create(null) || {};

The tiny edge-case bug this fixes will still exist in IE8, but it also means not having to ship a polyfill. An issue can be opened for adding the polyfill and it can happen later (or the issue can be closed whenever IE8 support is dropped).

EDIT: Also not sure if the tests are run in browsers. If so, we'll need a mechanism to skip the new test in IE8. Not familiar enough with the project to know offhand if one already exists...

@UziTech
Copy link
Member

UziTech commented Jun 28, 2018

I'm ok with the check for Object.create, I'm also ok with not supporting IE8

@Trott
Copy link
Contributor Author

Trott commented Jun 28, 2018

I'm ok with the check for Object.create, I'm also ok with not supporting IE8

I'll assume "not support IE8" is the most likely path until someone else weighs in to the contrary. :-D

@joshbruce
Copy link
Member

@styfle, @UziTech, et al: Given the data we have from #1215 and the fact that IE11 isn't supported by Microsoft anymore I'm pretty okay with not supporting IE8 as well.

Having said that, I do wonder what criteria we should set for supporting IE in general (more specifically JS engines)...feel like we almost had this conversation not too long ago.

@styfle
Copy link
Member

styfle commented Jun 29, 2018

@joshbruce There is some discussion of when to begin using newer JS features (namely ES6) in the following tickets:

I am fine with dropping support for IE 8 but we have to be careful about IE 11 because that still has sizable market share in enterprise environments (unfortunately).

@styfle styfle merged commit bcf1aed into markedjs:master Jun 30, 2018
Trott added a commit to Trott/io.js that referenced this pull request Jul 2, 2018
Float a patch on marked-0.4.0 that fixes a bug that breaks links that
are named the same as properties on the Object prototype.

Refs: markedjs/marked#1299
@styfle styfle added this to the 0.5.0 - Commonmark Compliance milestone Aug 15, 2018
This was referenced Apr 6, 2020
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
fix issues link references and prototypes
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.

4 participants