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-flexible-code-titles to list of plugins #1108

Merged
merged 3 commits into from Feb 10, 2023
Merged

Add remark-flexible-code-titles to list of plugins #1108

merged 3 commits into from Feb 10, 2023

Conversation

talatkuyuk
Copy link
Contributor

@talatkuyuk talatkuyuk commented Jan 22, 2023

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 a new plugin remark-flexible-code-titles to list of plugins

https://github.com/ipikuka/remark-flexible-code-titles

@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 Jan 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2023

Codecov Report

Merging #1108 (b961049) into main (bec44aa) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #1108   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          906       906           
=========================================
  Hits           906       906           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bec44aa...b961049. Read the comment docs.

Copy link
Member

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks for sharing @talatkuyuk!

Looks good, a few tips for the plugin.

  1. For NodeNext or Node16 module resolution export types needs to be set https://github.com/ipikuka/remark-flexible-code-titles/blob/688b55ac891a544a4f9485849bafb7f9bc3738cb/package.json#L6-L11 see https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing for more on this
  2. mdast and unist appear to be part of the public interface, if so it would be advisable to include their @types/ package in dependencies https://github.com/ipikuka/remark-flexible-code-titles/blob/688b55ac891a544a4f9485849bafb7f9bc3738cb/package.json#L54
  3. You could probably simplify the typings by noting that the plugin accepts an mdast Root node, see https://github.com/remarkjs/remark-validate-links/blob/38da293cf84ff3cfda8caf8f45321536cc58fa94/lib/index.js#L54 for an example, when that type is set type narrowing like isCodeNode (https://github.com/ipikuka/remark-flexible-code-titles/blob/688b55ac891a544a4f9485849bafb7f9bc3738cb/src/index.ts#L69-L71) could be handled by the visit function itself
  4. For debugging purposes, it can be useful to make most functions named functions, rather than arrow functions assigned to a variable

@talatkuyuk
Copy link
Contributor Author

talatkuyuk commented Jan 22, 2023

Thank you @ChristianMurphy for quick review and tips.

Number-1

For NodeNext or Node16 module resolution export types needs to be set.

In the dist/esmand dist/cjs directories in the package, the index.d.ts file are colocated with the index.js file. In that cases no need to set export types explicitly. The doc you refer says:

...it will look for a colocated declaration file. If you need to point to a different location for your type declarations, you can add a "types" import condition.

Number-2

mdast and unist appear to be part of the public interface, if so it would be advisable to include their @types/ package in dependencies

They are not a part of the public interface (only Plugin itself). I suppose the "@types/mdast": "^3.0.10" and "@types/unist": "^2.0.6" are better to stay in devDependencies.

Number-3

You could probably simplify the typings by noting that the plugin accepts an mdast Root node.

I added as you pointed out.

export const plugin: Plugin<[CodeTitleOptions?], Root>

but I receive typing errors:

Type 'Transformer<Node<Data>, Node<Data>>' is not assignable to type 'Transformer<Root, Root>'.
      Property 'children' is missing in type 'Node<Data>' but required in type 'Root'.

I tried to fix it but couldn't figure it out. Is there any plugin that uses the types Plugin, Transformer, Visitor and narrowed the Node to a specific node like Code, Paragraph, Literal etc. by visit function itself and written in fully typescript? I need support for it.

I needed the function isCodeNode in order to narrow the Node<Data> into Code to get/set the node.lang or node.meta without typing problem.

Number-4

For debugging purposes, it can be useful to make most functions named functions, rather than arrow functions assigned to a variable.

Arrow functions are anonymous, which means that they are not named. This is a problem if you want to debug because you will not be able to trace the name of the function. To solve this, you could use a feature introduced in ES2015-ES6 that is the function name inference. JavaScript can determine the arrow function name from its syntactic position: from the variable name that holds the function object. it is a good practice to use function name inference to name the arrow functions if you want to debug your code. Quoted from this article

Another stackoverflow reference for it.

Thank you for your review again, I have learnt a lot while digging them.

doc/plugins.md Outdated Show resolved Hide resolved
@talatkuyuk
Copy link
Contributor Author

Hi @wooorm, I made the change that you requested.

@talatkuyuk
Copy link
Contributor Author

Hi @ChristianMurphy,

About exporting types explicitly, I found the option in the tsconfig.json namely declarationDir that copies theindex.d.ts to the specified directory, in my case "./dist/types". Now, the index.d.ts is in the different directory from index.js file. So I set the export types explicitly in the package.json. Just for your information.

@talatkuyuk
Copy link
Contributor Author

Hi @ChristianMurphy,

About simplifying typings, I fixed the typescript error that I receive. Now, the plugin accepts the mdast Root node, and type narrowing to "Code" is handled by the visit function itself. I removed the isCodeNode. Thanks for the tip.

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @talatkuyuk!

Appreciate the updates and for sharing the plugin with the community.
This LGTM 👍

Some, completely optional, follow up ideas/suggestions.

  1. in tsconfig.json when module: node16 is set: moduleResolution, esModuleInterop, and importHelpers can be omitted (and probably should be) https://github.com/ipikuka/remark-flexible-code-titles/blob/fc0cf0b262911846aded817adc9b244f1743704f/tsconfig.json#L6-L9
  2. with mdast.Root being used now, @types/mdast should probably move to dependencies (see: https://github.com/ipikuka/remark-flexible-code-titles/blob/fc0cf0b262911846aded817adc9b244f1743704f/src/index.ts#L80 and https://github.com/ipikuka/remark-flexible-code-titles/blob/fc0cf0b262911846aded817adc9b244f1743704f/package.json#L52)
  3. In package.json for dependencies use a wide ^ range where possible. Pinning specific versions of dependencies can cause version conflicts and duplication in the node_modules folder. For example "unist-util-visit": "^4.1.2" could be widened to "unist-util-visit": "^4.0.0" to allow for further deduplication.
  4. TypeScript has a different/looser interpretation of SemVer, they can introduce breaking changes for libraries in minor releases. Depending on how you want to handle that, you could use a ~ to lock down a specific minor release in tests, or keep it ^ with the expectation that it may break every now and again https://github.com/ipikuka/remark-flexible-code-titles/blob/fc0cf0b262911846aded817adc9b244f1743704f/package.json#L70
  5. it looks like it could be safe to add sideEffects: false to package.json which helps bundlers like webpack with tree shaking (https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free)
  6. Unless you specifically need a script to run during the install phase (I don't see any currently) I'd recommend setting ignore-scripts=true in .npmrc to limit supply chain risks from upstream packages having unnecessary life cycle scripts.

@talatkuyuk
Copy link
Contributor Author

talatkuyuk commented Feb 2, 2023

Hi @ChristianMurphy, I implemented all your suggestions in version 1.1.1 . I have learnt a lot, thanks again.

@wooorm wooorm merged commit 20e7543 into remarkjs:main Feb 10, 2023
@github-actions

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Feb 10, 2023

Thank you Talat! Glad to hear Christian’s tips are useful! :)

I also see you are from Türkiye. I am so sorry for the terrible things happening there, and in Syria, right now. I hope you and your loved ones are okay!

@wooorm wooorm added 📚 area/docs This affects documentation 💪 phase/solved Post is done labels Feb 10, 2023
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Feb 10, 2023
@talatkuyuk
Copy link
Contributor Author

Hi Titus,

Thank you very much for your message with good wishes.

Me and my family are fine. We live quite a distance from the earthquake zone. We have no loss of life from my own and my wife's relatives. However, some relatives of our friends lost their lives and there were those whose houses were destroyed.

The dimensions of the earthquake disaster are huge. More than 100,000 lives are thought to have been lost, and the vast majority are still under the rubble. We are very sad. May God not let any nation experience such a calamity.

tylersmalley pushed a commit to tailscale-dev/tailscale-dev that referenced this pull request Jun 5, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [remark](https://remark.js.org)
([source](https://togithub.com/remarkjs/remark)) | [`^14.0.2` ->
`^14.0.3`](https://renovatebot.com/diffs/npm/remark/14.0.2/14.0.3) |
[![age](https://badges.renovateapi.com/packages/npm/remark/14.0.3/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/remark/14.0.3/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/remark/14.0.3/compatibility-slim/14.0.2)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/remark/14.0.3/confidence-slim/14.0.2)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>remarkjs/remark</summary>

###
[`v14.0.3`](https://togithub.com/remarkjs/remark/releases/tag/14.0.3)

[Compare
Source](https://togithub.com/remarkjs/remark/compare/14.0.2...14.0.3)

##### Misc

-   Rerelease types for changes in TypeScript

[remarkjs/remark#1162
- [`f6bd64e`](https://togithub.com/remarkjs/remark/commit/f6bd64e6)
Refactor `tsconfig`s for perf and strictness
- [`bb4c814`](https://togithub.com/remarkjs/remark/commit/bb4c8143) Add
improved docs on what this project is
by [@&#8203;BeLi4L](https://togithub.com/BeLi4L) in
[remarkjs/remark#1147
- [`bec44aa`](https://togithub.com/remarkjs/remark/commit/bec44aa0)
Update `tsconfig.json` to use node16 module resolution
by [@&#8203;ChristianMurphy](https://togithub.com/ChristianMurphy) in
[remarkjs/remark#1106
- [`f07f413`](https://togithub.com/remarkjs/remark/commit/f07f413f) Add
`ignore-scripts` to `.npmrc`
by [@&#8203;ChristianMurphy](https://togithub.com/ChristianMurphy) in
[remarkjs/remark#1103
- [`134ece2`](https://togithub.com/remarkjs/remark/commit/134ece2b)
Update Actions
by [@&#8203;ChristianMurphy](https://togithub.com/ChristianMurphy) in
[remarkjs/remark#1070
- [`974f893`](https://togithub.com/remarkjs/remark/commit/974f8936) Fix
internal types for TS 4.9

##### Plugins

- [`1e488d0`](https://togithub.com/remarkjs/remark/commit/1e488d0b) Add
`remark-ins` to list of plugins
by [@&#8203;talatkuyuk](https://togithub.com/talatkuyuk) in
[remarkjs/remark#1129
- [`e456dc5`](https://togithub.com/remarkjs/remark/commit/e456dc5b) Add
`remark-flexible-markers` to list of plugins
by [@&#8203;talatkuyuk](https://togithub.com/talatkuyuk) in
[remarkjs/remark#1126
- [`42114fc`](https://togithub.com/remarkjs/remark/commit/42114fc6) Add
`remark-flexible-paragraphs` to list of plugins
by [@&#8203;talatkuyuk](https://togithub.com/talatkuyuk) in
[remarkjs/remark#1120
- [`6aa638a`](https://togithub.com/remarkjs/remark/commit/6aa638ab) Add
`remark-flexible-containers` to list of plugins
by [@&#8203;talatkuyuk](https://togithub.com/talatkuyuk) in
[remarkjs/remark#1112
- [`20e7543`](https://togithub.com/remarkjs/remark/commit/20e75435) Add
`remark-flexible-code-titles` to list of plugins
by [@&#8203;talatkuyuk](https://togithub.com/talatkuyuk) in
[remarkjs/remark#1108
- [`32d6948`](https://togithub.com/remarkjs/remark/commit/32d69488) Add
`remark-cloudinary-docusaurus` to list of plugins
by [@&#8203;johnnyreilly](https://togithub.com/johnnyreilly) in
[remarkjs/remark#1090
- [`28aa8b9`](https://togithub.com/remarkjs/remark/commit/28aa8b9a)
update tests for changes in `mdast-util-to-markdown`
- [`9af1a87`](https://togithub.com/remarkjs/remark/commit/9af1a876) Add
`remark-code-title` to list of plugins
by [@&#8203;kevinzunigacuellar](https://togithub.com/kevinzunigacuellar)
in
[remarkjs/remark#1076
- [`0d1eb09`](https://togithub.com/remarkjs/remark/commit/0d1eb09a) Add
7 plugins to list of plugins
by [@&#8203;Xunnamius](https://togithub.com/Xunnamius) in
[remarkjs/remark#1064
- [`c7e8171`](https://togithub.com/remarkjs/remark/commit/c7e81713)
Remove deprecated `remark-jargon`
by [@&#8203;LunaticMuch](https://togithub.com/LunaticMuch) in
[remarkjs/remark#1059

**Full Changelog**:
remarkjs/remark@14.0.2...14.0.3

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/tailscale-dev/tailscale-dev).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS44Ny4xIiwidXBkYXRlZEluVmVyIjoiMzUuODcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants