chore: update to marked version 9 #474

robertIsaac · 2023-10-28T16:59:46Z

New features and enhancements

The property markedExtensions has been added to MarkdownModuleConfig allowing to use marked extensions when configuring MarkdownModule.

MarkdownModule.forRoot({
  markedExtensions: [gfmHeadingId()],
}),

⚠ Breaking changes

All options that were removed from marked has been deleted from this library too, see more at https://marked.js.org/using_advanced#options
Some methods now return Promise<string> instead of string, because marked is doing so
The srcRelativeLink input property is removed as the baseUrl option has been removed from marked, use https://www.npmjs.com/package/marked-base-url instead
The MarkdownPipe now returns Promise<string> instead of string and will need to be combined with async pipe to work correctly

jfcere · 2023-10-28T22:26:33Z

Hi @robertIsaac,

Thanks for the PR, I was expecting breaking changes, marked went from v5 to v9 in a few months only.

I'll review it and come back to you but at first glance it seems pretty descent!

UziTech · 2023-10-30T05:00:28Z

lib/src/markdown.service.ts

+ private useExtensions() {
+ if (this.extensions) {
+ for (const ext of this.extensions) {
+ marked.use(ext);


You may want to think about using a Marked instance to prevent adding the same extensions multiple times. Some extensions will do strange things when added multiple times.

import { Marked } from 'marked'; const marked = new Marked(); marked.use(...); ...

@UziTech instead of creating an instance of marked, we can just make sure that marked.use(...) is not called more than once with the extensions right?

lib/src/markdown.service.ts

lib/src/marked-options.ts

robertIsaac · 2023-10-30T20:25:24Z

hi @UziTech I have addressed your comments, please review again and resolve the conversation if my change address them correctly

robertIsaac · 2023-11-03T15:34:12Z

hi @jfcere do you plan to merge this soon?

jfcere · 2023-11-03T15:50:27Z

hi @jfcere do you plan to merge this soon?

Hi @robertIsaac,

First, let me say a big thank you for doing the migration, and also a special thanks to @UziTech for reviewing it!

Because this is a breaking change, this cannot be merged in the master branch so I will have to create a release branch for the next major release.

Also, the README.md file for documentation will have to be adjusted.

So, long story short, yes I will merge but this won't be available until Angular 17 is released which is due for next week according to their release schedule.

I might start working on the next release this weekend, if you are in a hurry I can release an alpha version with those changes without the Angular 17 migration.

robertIsaac · 2023-11-03T15:55:14Z

hi @jfcere do you plan to merge this soon?

Hi @robertIsaac,

First, let me say a big thank you for doing the migration, and also a special thanks to @UziTech for reviewing it!

Because this is a break change, this cannot be merged in the master branch so I will have to create a release branch for the next major release.

Also, the README.md file for documentation will have to be adjusted.

So, long story short, yes I will merge but this won't be available until Angular 17 is released which is due for next week according to their release schedule.

okay I will modify the README.md now if you wish
I understand that shouldn't be released now, but it should be merged (into master or release branch whatever you prefer, just tell me to change the target)
but I also want to make another PR to migrate everything to standalone but I'm afraid if I worked on it alongside this we might endup with conflicts

jfcere · 2023-11-03T15:56:45Z

okay I will modify the README.md now if you wish

Of course that would really be appreciated :)

robertIsaac · 2023-11-03T15:56:45Z

another thing I want to do, to add prettier (or dprint) for the project if it's okay for you
I don't like that the every file has different format

robertIsaac · 2023-11-03T16:17:00Z

okay I will modify the README.md now if you wish

Of course that would really be appreciated :)

done

removed from the examples all the options that were removed
added section for the new marked extension token
removed the part that we need to add node_modules/marked/marked.min.js to our scripts since it's not needed
I doubled checked in my application, I never added it and it's working

jfcere · 2023-11-04T19:03:52Z

Hi @robertIsaac,

I've created a new branch for the next release and changed the target branch for your PR. As there were changes to update mermaid.js there is a small conflict in both package.json and yarn.lock file.

I am going to review your changes afterward and this should be merged real soon in the next release branch.

a new token `MARKDOWN_EXTENSIONS` has been added to provide extensions to marked usage example ``` { provide: MARKDOWN_EXTENSIONS, useValue: [gfmHeadingId()], } ``` update `readme.md` file - removed from the examples all the options that were removed - added section for the new marked extension token - removed the part that we need to add node_modules/marked/marked.min.js to our scripts since it's not needed breaking change - all options that were removed from marked has been deleted from this library too, see more at https://marked.js.org/using_advanced#options - some methods now return `Promise<string>` instead of `string`, because marked is doing so - `srcRelativeLink` input is removed as the baseUrl option has been removed from marked, use https://www.npmjs.com/package/marked-base-url instead

robertIsaac · 2023-11-04T19:45:43Z

@jfcere done

jfcere · 2023-11-05T16:46:28Z

@robertIsaac there are a few things I'd like to modify in your PR instead of pointing out everything and asking for you to do the changes. So I either do the changes directly in your PR or merge it and do another PR to adjust some stuff.

But seeing that you have another PR based on this one, the changes will inevitably bring conflicts to your other PR... do you have a preference?

… rejection so it's possible to override it in the unit test it starts with ɵ so IDE doesn't provide it in autocomplete suggestions

robertIsaac · 2023-11-05T17:34:04Z

@jfcere @michaelfaith
I made the suggestions here
also I found one unit test failed because using Marked class, did a workaround to fix it, I have no idea how to mock it directly using jasmine, if you have a better idea to mock it please share

remove unused `loader` from the `MarkdownModuleConfig`

robertIsaac · 2023-11-05T17:42:07Z

uploading to coveralls is denied
I guess because the PR is made from my repo
do you have any suggestions what do ot @jfcere ?

jfcere · 2023-11-05T17:44:28Z

@jfcere @michaelfaith I made the suggestions here also I found one unit test failed because using Marked class, did a workaround to fix it, I have no idea how to mock it directly using jasmine, if you have a better idea to mock it please share

I just think that using a new instance of marked everytime is not necessary, only to call marked.use() once with the extensions, so this is a problem with the way the extensions are provided everytime in your implementation and I already have the fix... that is why I was saying I will either push modifications to your branch or merged it and do the changes afteward as your prefer otherwise it might end up in an endless chain of "do this.. no that" etc

demo/src/app/app.module.ts

robertIsaac · 2023-11-05T23:18:46Z

@jfcere @michaelfaith I made the suggestions here also I found one unit test failed because using Marked class, did a workaround to fix it, I have no idea how to mock it directly using jasmine, if you have a better idea to mock it please share

I just think that using a new instance of marked everytime is not necessary, only to call marked.use() once with the extensions, so this is a problem with the way the extensions are provided everytime in your implementation and I already have the fix... that is why I was saying I will either push modifications to your branch or merged it and do the changes afteward as your prefer otherwise it might end up in an endless chain of "do this.. no that" etc

okay do your modifications in my branch I don't mind

robertIsaac · 2023-11-05T23:20:24Z

@robertIsaac there are a few things I'd like to modify in your PR instead of pointing out everything and asking for you to do the changes. So I either do the changes directly in your PR or merge it and do another PR to adjust some stuff.

But seeing that you have another PR based on this one, the changes will inevitably bring conflicts to your other PR... do you have a preference?

sorry I just saw this comment
do the changes here so that we have one PR for the update
I will rebase the second PR once this merge is created

This reverts commit be713bf.

jfcere · 2023-11-08T02:16:56Z

This PR will be merged into a temporary branch on ngx-markdown before merging into the next release branch to...

Make adjustments as it is not possible to override the renderer anymore
Merge the release branch with this PR changes
Keep credits from the original PR author

* chore: update to marked version 9 (#474) * chore: update to marked version 9 * Update README.md * test: fix the Marked testing by providing it using Angular dependency rejection * feat: add `extensions` to the `MarkdownModuleConfig` * Revert "feat: add `extensions` to the `MarkdownModuleConfig`" * Re-add extensions to the MarkdownModuleConfig * Add missing unit tests --------- Co-authored-by: jfcere <[email protected]> * Add removed unit tests * Rename markdown-extentions file * Injection tokens uniformization * Add async pipe to MarkdownPipe demo and documentation * Remove marked.min.js from angular.json * Fix overriding of renderer functions * Fix named import lint error --------- Co-authored-by: robertIsaac <[email protected]> Co-authored-by: jfcere <[email protected]>

robertIsaac force-pushed the update-to-marked-9 branch 2 times, most recently from addc6f6 to 99feada Compare October 28, 2023 17:13

UziTech reviewed Oct 30, 2023

View reviewed changes

lib/src/markdown.service.ts Outdated Show resolved Hide resolved

UziTech reviewed Oct 30, 2023

View reviewed changes

lib/src/marked-options.ts Outdated Show resolved Hide resolved

robertIsaac force-pushed the update-to-marked-9 branch 2 times, most recently from cf4ddb7 to c3b23e3 Compare October 30, 2023 20:19

UziTech approved these changes Oct 30, 2023

View reviewed changes

robertIsaac force-pushed the update-to-marked-9 branch 2 times, most recently from a42791b to e0467bb Compare November 3, 2023 16:16

robertIsaac force-pushed the update-to-marked-9 branch from e0467bb to d277af4 Compare November 3, 2023 16:21

jfcere changed the base branch from master to version-17.0.0 November 4, 2023 19:01

robertIsaac force-pushed the update-to-marked-9 branch from d277af4 to b77382b Compare November 4, 2023 19:45

robertIsaac mentioned this pull request Nov 4, 2023

chore: migrate to standalone #476

Closed

Update README.md

ab50f84

test: fix the Marked testing by providing it using Angular dependency…

2e87734

… rejection so it's possible to override it in the unit test it starts with ɵ so IDE doesn't provide it in autocomplete suggestions

feat: add extensions to the MarkdownModuleConfig

be713bf

remove unused `loader` from the `MarkdownModuleConfig`

robertIsaac force-pushed the update-to-marked-9 branch from 722ebc6 to be713bf Compare November 5, 2023 17:35

jfcere reviewed Nov 5, 2023

View reviewed changes

demo/src/app/app.module.ts Outdated Show resolved Hide resolved

jfcere added 3 commits November 7, 2023 19:33

Revert "feat: add extensions to the MarkdownModuleConfig"

e8cdcad

This reverts commit be713bf.

Re-add extensions to the MarkdownModuleConfig

a64ae56

Add missing unit tests

65d5266

jfcere changed the base branch from version-17.0.0 to pr474 November 8, 2023 02:03

jfcere merged commit dbf7e49 into jfcere:pr474 Nov 8, 2023
1 check failed

robertIsaac deleted the update-to-marked-9 branch November 8, 2023 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update to marked version 9 #474

chore: update to marked version 9 #474

robertIsaac commented Oct 28, 2023 •

edited by jfcere

Loading

jfcere commented Oct 28, 2023

UziTech Oct 30, 2023

robertIsaac Oct 30, 2023

jfcere Nov 5, 2023 •

edited

Loading

robertIsaac commented Oct 30, 2023

robertIsaac commented Nov 3, 2023

jfcere commented Nov 3, 2023 •

edited

Loading

robertIsaac commented Nov 3, 2023 •

edited

Loading

jfcere commented Nov 3, 2023

robertIsaac commented Nov 3, 2023 •

edited

Loading

robertIsaac commented Nov 3, 2023 •

edited

Loading

jfcere commented Nov 4, 2023 •

edited

Loading

robertIsaac commented Nov 4, 2023

jfcere commented Nov 5, 2023 •

edited

Loading

robertIsaac commented Nov 5, 2023

robertIsaac commented Nov 5, 2023

jfcere commented Nov 5, 2023 •

edited

Loading

robertIsaac commented Nov 5, 2023

robertIsaac commented Nov 5, 2023 •

edited

Loading

jfcere commented Nov 8, 2023 •

edited

Loading

chore: update to marked version 9 #474

chore: update to marked version 9 #474

Conversation

robertIsaac commented Oct 28, 2023 • edited by jfcere Loading

New features and enhancements

⚠ Breaking changes

jfcere commented Oct 28, 2023

UziTech Oct 30, 2023

Choose a reason for hiding this comment

robertIsaac Oct 30, 2023

Choose a reason for hiding this comment

jfcere Nov 5, 2023 • edited Loading

Choose a reason for hiding this comment

robertIsaac commented Oct 30, 2023

robertIsaac commented Nov 3, 2023

jfcere commented Nov 3, 2023 • edited Loading

robertIsaac commented Nov 3, 2023 • edited Loading

jfcere commented Nov 3, 2023

robertIsaac commented Nov 3, 2023 • edited Loading

robertIsaac commented Nov 3, 2023 • edited Loading

jfcere commented Nov 4, 2023 • edited Loading

robertIsaac commented Nov 4, 2023

jfcere commented Nov 5, 2023 • edited Loading

robertIsaac commented Nov 5, 2023

robertIsaac commented Nov 5, 2023

jfcere commented Nov 5, 2023 • edited Loading

robertIsaac commented Nov 5, 2023

robertIsaac commented Nov 5, 2023 • edited Loading

jfcere commented Nov 8, 2023 • edited Loading

robertIsaac commented Oct 28, 2023 •

edited by jfcere

Loading

jfcere Nov 5, 2023 •

edited

Loading

jfcere commented Nov 3, 2023 •

edited

Loading

robertIsaac commented Nov 3, 2023 •

edited

Loading

robertIsaac commented Nov 3, 2023 •

edited

Loading

robertIsaac commented Nov 3, 2023 •

edited

Loading

jfcere commented Nov 4, 2023 •

edited

Loading

jfcere commented Nov 5, 2023 •

edited

Loading

jfcere commented Nov 5, 2023 •

edited

Loading

robertIsaac commented Nov 5, 2023 •

edited

Loading

jfcere commented Nov 8, 2023 •

edited

Loading