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

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

Closed

Conversation

Robot-Inventor
Copy link

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Added remark-auto-ads to the documentation

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Mar 4, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChristianMurphy
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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

Yes please

@Robot-Inventor
Copy link
Author

Understood. thank you!

This comment has been minimized.

@wooorm wooorm added the 🤷 no/invalid This cannot be acted upon label Mar 6, 2024
@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤷 no/invalid This cannot be acted upon 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

None yet

5 participants