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

refactor(remark-plugin-npm2yarn): migrate package to TS #5931

Merged
merged 9 commits into from
Nov 12, 2021

Conversation

duanwilliam
Copy link
Contributor

@duanwilliam duanwilliam commented Nov 12, 2021

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 12, 2021
@netlify
Copy link

netlify bot commented Nov 12, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: b894444

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/618e6636d6c1a7000844b3de

😎 Browse the preview: https://deploy-preview-5931--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Nov 12, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 85
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5931--docusaurus-2.netlify.app/

@duanwilliam duanwilliam marked this pull request as draft November 12, 2021 07:50
@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Nov 12, 2021
@Josh-Cena
Copy link
Collaborator

Wow, thanks! LGTM 👍 It's just reconciling the types for the config file that's left.

Comment on lines 87 to 88
const {children: result} =
(transformer(node.children[index], _) as Parent) ?? {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

kinda hacky to return a dummy node with the list of children and then destructuring it but it's the minimal change to make transformer a valid unified Transformer type. don't know if that's alright though

Copy link
Collaborator

@Josh-Cena Josh-Cena Nov 12, 2021

Choose a reason for hiding this comment

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

Actually can we use unist-util-visit for this? I'm not sure why we don't now

Like this: https://github.com/smack0202/luxdocs/blob/main/src/remark/video.js

@Josh-Cena Josh-Cena marked this pull request as ready for review November 12, 2021 12:29
@Josh-Cena
Copy link
Collaborator

Thanks, made some refactors so things look better. All the type assertions & typeguards are pretty justified. Since the behavior is still the same and we don't have many edge cases for this, I'll merge

@Josh-Cena Josh-Cena merged commit 1e725a1 into facebook:main Nov 12, 2021

// To continue supporting `require('npm2yarn')` without the `.default` ㄟ(▔,▔)ㄏ
// TODO change to export default after migrating to ESM
export = plugin;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's annoying me due to not working with Babel, isn't there a better way to handle that interop problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is:

// To trick Babel & tsc
export default plugin;
// To actual do the export
module.exports = plugin;

This will be transpiled as:

exports.default = plugin;
module.exports = plugin;

Because exports is an alias for module.exports, the last line decouples the exports reference from module.exports and makes the second last line useless. However this makes everyone happy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants