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

JS tooling: upgrade to babel 7 #13873

Merged
merged 4 commits into from Dec 4, 2018
Merged

JS tooling: upgrade to babel 7 #13873

merged 4 commits into from Dec 4, 2018

Conversation

davkal
Copy link
Contributor

@davkal davkal commented Oct 29, 2018

  • upgrade from babel 6 to 7
  • use babel/preset-env and useBuiltIns to govern what is included from babel polyfill
  • replace ts-loader from hot-reload webpack config, TS compilation now being done by babel plugin

Seems to work ok for yarn build and yarn dev. But with yarn start I get errors when accessing a dashboard: TypeError: Cannot read property 'meta' of undefined which seems to come from dashnav.ts:

  /** @ngInject */
  constructor(private $scope, private dashboardSrv, private $location, public playlistSrv) {
    appEvents.on('save-dashboard', this.saveDashboard.bind(this), $scope);

    if (this.dashboard.meta.isSnapshot) {

Somehow the instantiation is not working (dashboard is missing, which is supplied via an angular directive).

@torkelo
Copy link
Member

torkelo commented Oct 29, 2018

if dashboard is null there is could be that somehow pre assign bindings is not enabled .

https://github.com/grafana/grafana/blob/master/public/app/app.ts#L70

not sure why hot reload should cause issues with this binding flag

@davkal
Copy link
Contributor Author

davkal commented Oct 29, 2018

The bundle implications are negligible:

Before:

     vendor.d9d56360a977b6c2316b.js    3.39 MiB       0  [emitted]  [big]  vendor

After:

     vendor.6af89965262332b9c799.js    3.38 MiB       0  [emitted]  [big]  vendor

@@ -47,25 +47,31 @@ module.exports = merge(common, {
module: {
rules: [
{
test: /\.tsx?$/,
test: /\.(j|t)sx?$/,
Copy link
Member

Choose a reason for hiding this comment

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

@davkal what's the reasoning behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted the config to be the same as in react-hot-reloader README in case I overlooked something. jsx? won't be included in this PR. Thanks for highlighting it out though.

@torkelo
Copy link
Member

torkelo commented Nov 23, 2018

This was tricky.

Somehow babel is doing stuff that is not compatible or the same as typescript output. Babel is setting class properties to undefined. which breaks angularjs bindings that pre assigns properties before calling constructor.

Possibly related issues
babel/babel#7644
babel/babel#7074
babel/babel#5854
babel/babel#8682

Not sure how to fix this. Tried some suggestions in those issues without success.

properties to void 0. This breaks angularjs preAssignBinding which
applies bindings to this before constructor is called. Fixed by
using fork of babel plugin.

babel/babel#8417
@torkelo
Copy link
Member

torkelo commented Nov 23, 2018

Pushed a fix using fork of the problematic babel plugin,
fd71abc

Also investigated the why the type warnings show up (Bug in webpack where there currently is no workaround other than filtering them out), they are filtered out in normal webpack watch mode using stats warningFilter, but this seems to be ignored by webpack dev server:
webpack/webpack-dev-server#1557

@davkal davkal changed the title [WIP] JS tooling: upgrade to babel 7 JS tooling: upgrade to babel 7 Dec 3, 2018
@davkal
Copy link
Contributor Author

davkal commented Dec 3, 2018

LGTM now.

@torkelo torkelo merged commit 6c46a1c into master Dec 4, 2018
@torkelo torkelo deleted the davkal/babel-7 branch December 4, 2018 20:16
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

3 participants