Skip to content

Fix to work with rollup-plugin-typescript2#147

Merged
gregberge merged 2 commits into
gregberge:masterfrom
vwxyutarooo:master
Aug 16, 2018
Merged

Fix to work with rollup-plugin-typescript2#147
gregberge merged 2 commits into
gregberge:masterfrom
vwxyutarooo:master

Conversation

@vwxyutarooo

@vwxyutarooo vwxyutarooo commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

Resolve #127

@codecov

codecov Bot commented Jul 27, 2018

Copy link
Copy Markdown

Codecov Report

Merging #147 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage    88.7%   88.76%   +0.05%     
==========================================
  Files          26       26              
  Lines         434      436       +2     
  Branches      127      110      -17     
==========================================
+ Hits          385      387       +2     
- Misses         36       38       +2     
+ Partials       13       11       -2
Impacted Files Coverage Δ
packages/rollup/src/index.js 100% <100%> (ø) ⬆️
packages/core/src/h2x/removeDimensions.js 83.33% <0%> (ø) ⬆️
packages/core/src/h2x/replaceAttrValues.js 72.72% <0%> (ø) ⬆️

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 c7ab92b...5fb1c17. Read the comment docs.

@gregberge

Copy link
Copy Markdown
Owner

@vwxyutarooo thanks for your research and your PR!

In fact we have another problem. The Babel transform should have its own configuration and not use the .babelrc of the project. We have to use babel-preset-env without any target, even Node 6 is not good.

By modifying the .babelrc you are modifying the way the whole project is build that's why we have to revert this change and specify a custom Babel config in https://github.com/smooth-code/svgr/pull/147/files#diff-8fab4aab82ae41a7c435ac29a5b56e25

@gregberge gregberge left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

  • Remove change in Babel config
  • Add a specific Babel config (only @babel/preset-env) in the rollup plugin

@vwxyutarooo

Copy link
Copy Markdown
Contributor Author

@neoziro thanks for checking my PR! Now I've fixed common .babelrc problem.

@gregberge

Copy link
Copy Markdown
Owner

Thanks! I am sorry maybe I wasn't clear in my explanations. A .babelrc impact the build, in this specific case we need to specify the Babel configuration directly in the code. In the call of babel transform just here https://github.com/smooth-code/svgr/pull/147/files#diff-8fab4aab82ae41a7c435ac29a5b56e25R59

@vwxyutarooo

Copy link
Copy Markdown
Contributor Author

Sorry for my miss understood. Your suggestion changes result of source code from SVG right? However, What this PR needed is remove async/await syntax from packages/rollup/lib/index.js actually. Because rollup-plugin-typescript-2 uses object-hash to create cache directory (key) with rollup config file at here. Then problem is the object-hash. This doesn't able to handle async function syntax. Issue is here: Support async functions.
Ways to solve this is that

  1. Fix object-hash to support async/await syntax
  2. Avoid to use object-hash from rollup-plugin-typescript-2
  3. Provide rollup-plugin without async/await (This PR)

Because 1 or 2 seems take quite a while, Is it possible to provide rollup plugin with condition 3 until 1 or 2 fixes?


return {
name: 'svgr',
async transform(data, id) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@vwxyutarooo so the problem is here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exactly. However things seems come to fixable on the rollup-plugin-typescript2 issue. So I'll revert the commit around .babelrc anyway!

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

OK nice!

@gregberge gregberge merged commit 4b3737e into gregberge:master Aug 16, 2018
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.

2 participants