remarkjs / remark

markdown processor powered by plugins part of the @unifiedjs collective
https://remark.js.org
MIT License
7.66k stars 358 forks source link

Add `remark-auto-ads` to list of plugins #1294

Closed Robot-Inventor closed 7 months ago

Robot-Inventor commented 8 months ago

Initial checklist

Description of changes

Added remark-auto-ads to the documentation

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (d993c7f) to head (ee954cb). Report is 3 commits behind head on main.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1294 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 6 6 Lines 142 142 ========================================= Hits 142 142 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ChristianMurphy commented 7 months ago

Welcome @Robot-Inventor! 👋 Thanks for sharing.

A few thoughts:

  1. I'd broadly recommend against placing ads in the middle of an article, it can be very disruptive for readers, sidebar placement or at the end of the article still get views but without less disruption.
  2. this plugin appears to work exclusively with HTML, it would probably be better as a rehype plugin on the hast tree. More on the different ASTs https://unifiedjs.com/learn/
  3. The plugin usage as currently documented/tested would inject the script tag multiple times on the page, this would cause multiple pauses while rendering, usually the script tag should be at the end of the page or have the defer attribute https://github.com/Robot-Inventor/remark-auto-ads/blob/5e8ea804f42e39a8e238d7a39cd6a757c348f4fa/src/index.test.ts#L12-L14 (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script)
  4. When the tsconfig option module is set to node16 or nodenext the esModuleInterop module should be removed, it will add unneeded cruft https://github.com/Robot-Inventor/remark-auto-ads/blob/5e8ea804f42e39a8e238d7a39cd6a757c348f4fa/tsconfig.json#L74
  5. Avoid augmenting unist Node it will impact usage of other ASTs like HAST/MDX, this could be avoided entirely by working on a HAST tree (see first point). https://github.com/Robot-Inventor/remark-auto-ads/blob/5e8ea804f42e39a8e238d7a39cd6a757c348f4fa/src/index.ts#L40-L48
  6. Position matters with options, the first parameter is the options not second or third. The more accurate type would be Plugin<[RemarkAutoAdsOptions], Root> https://github.com/Robot-Inventor/remark-auto-ads/blob/5e8ea804f42e39a8e238d7a39cd6a757c348f4fa/src/index.ts#L55
  7. Wider version ranges (usually a full major) allow package managers to more effectively de-duplicate dependencies consider a ^6.0.0 range. https://github.com/Robot-Inventor/remark-auto-ads/blob/5e8ea804f42e39a8e238d7a39cd6a757c348f4fa/package.json#L40
Robot-Inventor commented 7 months ago

@ChristianMurphy Thank you for your detailed review and suggestions!

  1. If you just want to place ads at the end of an article or in the sidebar, you do not need to use a plugin. Also, since it is relatively common (at least in my country) to insert ads in the middle of an article, I don't think it is much of a problem.
  2. and 5. Surely it would be better to make it a rehype plugin and use hast. I'll see if I can rewrite it to rehype.
  3. I am unsure if modifications to change the script insertion position or add defer attributes are allowed, so I do not want to change the ad code. This type of modification is neither allowed nor prohibited, so it is safer not to change it. (https://support.google.com/adsense/answer/1354736?hl=en)
  4. Removed esModuleInterop option
  5. See 2
  6. Fixed the type definition
  7. My repository uses the renovate bot so it may be "updated" if I change the version range. I'll look into the renovate configuration.

I will try to see if I can rewrite it to the rehype plugin, so you may close this PR. Alternatively, I will report back to you if the rewrite is successful.

ChristianMurphy commented 7 months ago

Also, since it is relatively common (at least in my country) to insert ads in the middle of an article, I don't think it is much of a problem.

Being a good idea and being common are two unrelated things. 🙂 Yes it is common in many countries. That doesn't change that it is disruptive to the reading flow and a bad user experience. 🤷

My repository uses the renovate bot so it may be "updated" if I change the version range. I'll look into the renovate configuration

renovate has a preset for this you can add to the extends section ":preserveSemverRanges" https://docs.renovatebot.com/presets-default/#preservesemverranges

ChristianMurphy commented 7 months ago

Another note, plug-and-play package managers, notably pnpm and yarn will error if type dependencies which are part of the public interface, are not included in the dependencies section of the package. unified and mdast could trigger this condition https://github.com/Robot-Inventor/remark-auto-ads/blob/1846a79e662d7a5a85c208535f3a5e4a0d9898f6/src/index.ts#L2-L4

remcohaszing commented 7 months ago

Being a good idea and being common are two unrelated things. 🙂 Yes it is common in many countries. That doesn't change that it is disruptive to the reading flow and a bad user experience. 🤷

On a personal level I fully agree. However, this choice is probably often some management decision. I think this plugin can be useful to developers who need to implement it.

Robot-Inventor commented 7 months ago

That doesn't change that it is disruptive to the reading flow and a bad user experience. 🤷

Yes, as a reader, I don't really like advertisements in articles.🙁 But you have to balance user experience and revenue.

This plugin allows you to set the number of paragraphs to insert ads, so if you increase this value, you can balance user experience and revenue.

Robot-Inventor commented 7 months ago

Being a good idea and being common are two unrelated things. 🙂 Yes it is common in many countries. That doesn't change that it is disruptive to the reading flow and a bad user experience. 🤷

On a personal level I fully agree. However, this choice is probably often some management decision. I think this plugin can be useful to developers who need to implement it.

Yes, it's mostly a management decision. This plugin maintains the user experience compared to Google's auto ads, which are difficult to control.

Robot-Inventor commented 7 months ago

I successfully rewrote it to rehype plugin! (Also, I changed the renovate config and fixed dependencies section)

https://github.com/Robot-Inventor/rehype-auto-ads

The next step is to close this pull request and resubmit it to the rehype repository, right?

ChristianMurphy commented 7 months ago

The next step is to close this pull request and resubmit it to the rehype repository, right?

Yes please

Robot-Inventor commented 7 months ago

Understood. thank you!

github-actions[bot] commented 7 months ago

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.