-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
chore: migrate to standalone #476
Conversation
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
this will make it possible to completely remove NgModule
with the first two options - Convert all components, directives and pipes to standalone - Remove unnecessary NgModule classes add option ``"newlines-between": "never"` to eslint rule import/order because the angular script that removed the imports left empty lines reformatted some files as the angular script has changed some files styles tried to minimize the changes
with Bootstrap the application using standalone APIs - clean the generated content - refactor the code into `app.config.ts` to match new angular projects
- mark `MarkdownModule` as deprecated - remove loader option, as it's not needed
should only be merged after #474 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this update. I just made a couple of recommendations.
{ | ||
provide: MARKDOWN_EXTENSIONS, | ||
useValue: [gfmHeadingId()], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this injection token, one option that's slightly more aligned with the new provide*
function pattern, is to use a with*
function that you pass in as a second parameter to the provide
function, which allows users to add additional options like this use case. So, for example, a user could do this in their app.config
providers: [
provideMarkdown({ ...markdownConfig }, withExtensions([gfmHeadingId()]),
]
The withExtensions
function would set up the appropriate injection token for the provideMarkdown
to use. You can see an example of this in the article I linked (search withColor
): https://www.angulararchitects.io/en/blog/patterns-for-custom-standalone-apis-in-angular/
Angular's new provideRouter
function has several with*
feature functions, if you want to use that for inspiration as well. E.g. withPreloading
https://angular.io/api/router/withPreloading
You could potentially do the same with several of the optional features that are embedded in the Markdown config, like moving Clipboard options into a withClipboard
feature function. But that may be a bit more of an overhaul than @jfcere may be comfortable with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be consistent
Either to add the extensions to the configuration interface
Or to use with for everything
What do you think @jfcere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main difference with the injection token, though, is that it's not integrated into the config interface right now. So it's already sort of its own thing, and could be converted to a with
function. But I agree, it would be nice to lean all the way into that pattern for optional features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be consistent Either to add the extensions to the configuration interface Or to use with for everything What do you think @jfcere
I agree that it should be consistent, honestly I never had time to really dive into the standalone components architecture so I am not sure which approach is best.
I always favored having providers within the markdownModuleConfig
so that it kinda regroup everything that is used by the ngx-markdown library instead of adding providers in the providers
property of the module and ending up with a bunch of stuff you are not clearly sure it is used by what module.
That being said, I do think that the MARKDOWN_EXTENSIONS
should be an option within the markdownModuleConfig
but this should be a comment for the previous PR #474. So I would be confortable with not having a bunch of withSomething
functions for now.
Does it make sense to you guys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
I will do it right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfcere I totally agree on having the self-contained config vs having a bunch of separate providers. But I feel like, when using the new provide*
function paradigm, the with*
feature functions feel a bit more natural, than passing in a large config to the function. So, for example, rather than having this
function markedOptionsFactory(anchorService: AnchorService): MarkedOptions {
const renderer = new MarkedRenderer();
// fix `href` for absolute link with fragments so that _copy-paste_ urls are correct
renderer.link = (href: string, title: string, text: string) => {
return MarkedRenderer.prototype.link.call(renderer, anchorService.normalizeExternalUrl(href), title, text) as string;
};
return { renderer };
}
provideMarkdown({
markedOptions: {
provide: MarkedOptions,
useFactory: markedOptionsFactory,
deps: [AnchorService],
},
clipboardOptions: {
provide: ClipboardOptions,
useValue: {
buttonComponent: ClipboardButtonComponent,
},
},
sanitize: SecurityContext.NONE,
}),
you could have something like this
function createMarkedOptions: MarkedOptions {
const anchorService: AnchorService = inject(AnchorService);
const renderer = new MarkedRenderer();
// fix `href` for absolute link with fragments so that _copy-paste_ urls are correct
renderer.link = (href: string, title: string, text: string) => {
return MarkedRenderer.prototype.link.call(renderer, anchorService.normalizeExternalUrl(href), title, text) as string;
};
return { renderer };
}
provideMarkdown(
createMarkedOptions(),
withClipboard({
buttonComponent: ClipboardButtonComponent,
}),
),
Internally those functions could still create the same config object, but it would remove the burden from the user to have to construct it. I don't have a strong preference, but the latter feels a bit more ergonomic and in line with the new direction the framework is going with their APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So taking the exemple...
provideMarkdown({
markedOptions: {
provide: MarkedOptions,
useFactory: markedOptionsFactory,
deps: [AnchorService],
},
markedExtensions {
provide: MarkedExtensions,
useValue: [gfmHeadingId()],
},
clipboardOptions: {
provide: ClipboardOptions,
useValue: { buttonComponent: ClipboardButtonComponent },
},
sanitize: SecurityContext.NONE,
}),
You would end up with 4 with*
functions as such (one for each property of the MarkdownConfig
) ?
provideMarkdown(
withMarkedOptions(getMarkedOptions()),
withMarkedExtensions([gfmHeadingId()]),
withClipboardOptions({ buttonComponent: ClipboardButtonComponent }),
withSanitize(SecurityContext.NONE),
),
Or this could probably be in the middle such as...
provideMarkdown({
markedOptions: getMarkdownOptions(),
markedExtensions: [gfmHeadingId()],
clipboardOptions: { buttonComponent: ClipboardButtonComponent },
sanitize: SecurityContext.NONE,
}),
I am an OCD person where having a bunch of with*
functions doesn't seem like an absolute necessity and I would be comfortable with the last one, I think it's pretty straightforward and it does remove the burden on the user to construct the whole provider object (as this could be done internally).
What you guys think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I was thinking the core MarkedOptions
would be the first parameter of the provideMarkdown
function, since those are core functionality. And then the other types of feature options would be encapsulated by with
functions.
e.g.
provideMarkdown(
getMarkedOptions(),
withMarkedExtensions([gfmHeadingId()]),
withClipboardOptions({ buttonComponent: ClipboardButtonComponent }),
withSanitize(SecurityContext.NONE),
),
It's just making a slight delineation between core function and additional features. But the other way could work too.
MarkdownService, | ||
markdownModuleConfig && markdownModuleConfig.loader || [], | ||
markdownModuleConfig && markdownModuleConfig.clipboardOptions || [], | ||
markdownModuleConfig && markdownModuleConfig.markedOptions || [], | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you should be able to replace what's in the providers
array with the new provideMarkdown
function, so that this isn't duplicated.
I think it is worth mentioning that this PR is based on another one that hasn't been reviewed yet. I am not planning to review this one before the other PR has been merged as the base PR will require some small adjustments. |
I only noticed 2 hours after merging the release branch that it automatically closed the PR because the branch was deleted. Feel free to reopen a new PR based on master but expect a lot of conflicts. |
add
provideMarkdown
as a new way to configure ngx-markdownrun ng generate @angular/core:standalone with the first two options
add option
"newlines-between": "never"
to eslint rule import/orderbecause the angular script that removed the imports left empty lines
reformatted some files as the angular script has changed some files styles
tried to minimize the changes
run ng generate @angular/core:standalone with Bootstrap the application using standalone APIs
app.config.ts
to match new angular projectsmove routing to the new standalone
mark
MarkdownModule
as deprecatedremove loader option, as it's not needed
breaking change
removing the loader option from
MarkdownModuleConfig
, you can safely delete it as it was unnecessaryfixes #458