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

fix(module-type-aliases): add svg declaration #5945

Merged
merged 4 commits into from
Nov 15, 2021
Merged

fix(module-type-aliases): add svg declaration #5945

merged 4 commits into from
Nov 15, 2021

Conversation

MisterFISHUP
Copy link
Contributor

Motivation

Try to fix #3424 (comment) by adding module declarations in packages/docusaurus-module-type-aliases/src/index.d.ts:

Currently, if one uses import ending with .module.scss, .scss, .svg in a .tsx file, VSCode (ESLint) will display errors like:

Cannot find module '[some_file].module.scss' or its corresponding type declarations.
Cannot find module '[some_file].scss' or its corresponding type declarations.
Cannot find module '[some_file].svg' or its corresponding type declarations.

These errors don't appear in .js files.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Just tested with yarn run start (after cd website), yarn prettier, yarn lint, and yarn test.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 13, 2021
@armano2
Copy link
Contributor

armano2 commented Nov 14, 2021

i can agree with svg being in there, but user should define it themself for scss ans sass

@Josh-Cena
Copy link
Collaborator

Thanks, I agree with @armano2—afaik we don't have SASS & SCSS loaders OOTB, do we? Do they work without any extra configuration?

@netlify
Copy link

netlify bot commented Nov 14, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 6c677b9

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

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

@github-actions
Copy link

github-actions bot commented Nov 14, 2021

⚡️ Lighthouse report for the changes in this PR:

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

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

@MisterFISHUP
Copy link
Contributor Author

Thanks @Josh-Cena @armano2. Tell me if I should remove the scss/module.scss parts or move them to another file. As I mentioned in the description, any imports ending with .scss or .module.scss in a .tsx file will have Cannot find module... errors, even I set up the project with ts, scss supports.

@Josh-Cena
Copy link
Collaborator

@MisterFISHUP You could do it in your own project: add a types.d.ts file in src and declare the scss and module.scss modules.

@MisterFISHUP MisterFISHUP changed the title fix(module-type-aliases): add svg, scss, module.scss, module.sass fix(module-type-aliases): add svg declaration Nov 14, 2021
@MisterFISHUP
Copy link
Contributor Author

You could do it in your own project: add a types.d.ts file in src and declare the scss and module.scss modules.

That's what I did 😄. Anyway, thanks for your reply @Josh-Cena. I also changed the title of the PR.

@MisterFISHUP
Copy link
Contributor Author

MisterFISHUP commented Nov 14, 2021

@Josh-Cena I'm not sure if I did this correctly, so I would like to ask your opinion before making the commit:

// in `packages/docusaurus-module-type-aliases/src/index.d.ts`
declare module '*.svg' {
  import type { ComponentType, SVGProps } from 'react';

  const ReactComponent: ComponentType<SVGProps<SVGSVGElement>>;

  export default ReactComponent;
}

Thank you for your time

@Josh-Cena
Copy link
Collaborator

Yeah, I think that looks better

@Josh-Cena Josh-Cena merged commit 7e955e2 into facebook:main Nov 15, 2021
@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Nov 15, 2021
@MisterFISHUP MisterFISHUP deleted the declare-missing-modules branch November 15, 2021 19:08
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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v2] Usage with TypeScript
4 participants