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

Change SVGProps from import to import type #853

Merged
merged 3 commits into from May 9, 2023
Merged

Change SVGProps from import to import type #853

merged 3 commits into from May 9, 2023

Conversation

johncch
Copy link
Contributor

@johncch johncch commented Apr 9, 2023

Summary

This PR changes SVGProp from value import to type import when the --typescript flag is supplied.

Here's an example of the change:

image

The change is quite straightforward. It adds a importKind parameter to the getOrCreateImport method so that both when importing the react and react-native versions of SVGProps, we can create the ImportDeclaration node with the right importKind parameter.

This addresses the warning for verbatimModuleSyntax flag in Typescript 5.0, seen below[they shipped the --verbatimModuleSyntax flag]

image

Note: this PR is branched off f20a753 because the build was failing in the newer commits.

This PR addresses #842

Test plan

I ran the jest tests and inspected the generated output to make sure they are correct.

I ran the updated script against my folder of SVGs and the Typescript compiler was happy with them.

@vercel
Copy link

vercel bot commented Apr 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svgr ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 20, 2023 11:30pm

@gregberge
Copy link
Owner

Thanks @johncch, I know it is not what you are fixing, but tests are actually broken. Could you please try to fix it? Else I will give a look when I have a moment.

@johncch
Copy link
Contributor Author

johncch commented Apr 11, 2023

Hi @gregberge, this is what I found:

The build is failing because it is not able to find the type definition file for svgr/core:
image

This only happens on a clean build. On the second build, since the dist folder already exists, it will just use the one that is built in the previous run. I replicated it and verified it by running this command in the CLI to delete all the dist folders before rebuilding it.

find packages -name dist -exec rm -rf {} \;
# simply delete all the folders name dist inside the packages folder

I can fix it locally and verified that the error stops occurring by adding svgr/core into the devDependencies. However, I noticed that you explicitly removed the svgr/core package from peerDependencies in this commit

I'm assuming that your intention is to not have the plugin-jsx package pull from NPM and pull from the local folder instead. If that's the case, may I suggest adding this as a fix?

  "devDependencies": {
    "@svgr/core": "file:../core"
  }

If you feel good about this I'll add a commit to this PR.

@gregberge
Copy link
Owner

Hello @johncch, could you rebase, with pnpm everything is fixed. Then I could merge it and make a new release.

…g is present

Accomplished this by adding `importKind` parameter to the `getOrCreateImport` method
@johncch
Copy link
Contributor Author

johncch commented Apr 20, 2023

Ok - rebased. I think it should be good. The tests run locally for me.

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #853 (ae94ae3) into main (428b0c7) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #853      +/-   ##
==========================================
+ Coverage   92.82%   92.87%   +0.04%     
==========================================
  Files          32       32              
  Lines         753      758       +5     
  Branches      250      253       +3     
==========================================
+ Hits          699      704       +5     
  Misses         53       53              
  Partials        1        1              
Impacted Files Coverage Δ
...el-plugin-transform-svg-component/src/variables.ts 99.15% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gregberge gregberge merged commit 095f021 into gregberge:main May 9, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants