-
-
Notifications
You must be signed in to change notification settings - Fork 400
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
Reuse babel options and specify polyfills for karma explicitely #1456
Conversation
babelOptions.js
Outdated
plugins: [ | ||
['@babel/proposal-class-properties', {loose: true}], | ||
['@babel/transform-runtime', {useESModules}], | ||
['@babel/plugin-proposal-export-namespace-from'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this plugin actually used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I searched for export * as
- nothing found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can remove it
@@ -0,0 +1,9 @@ | |||
exports.getBabelOptions = ({useESModules}) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you remember why we didn't use babel.config.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, but I would guess the useESModules option is dynamic, that's why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it can be used only for rollup. There is no difference between inlined and imported helpers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using the option for rollup only is fine, it's still better than having the entire babel config duplicted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, babel 7.13 does not require useESModules flag anymore. Right version is detected automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then we can remove the flag along with upgrading babel, probably better separately
…true}] the spread is doing a different thing
tests/utils.js
Outdated
return [...style.sheet.cssRules] | ||
const rulesArr = [] | ||
const {cssRules} = style.sheet | ||
for (const key in cssRules) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better for(;;)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hah, for in
failed in safari ... good catch
Released in v10.6.0 |
Fixes #1455
I was looking into why we didn't catch that with tests, it turned out we load all polyfills in karma, but as I was looking I made the babel config reusable, it didn't change anything but still good to have.