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

reduce code generated of plugin-proposal-decorators #6965

Closed
simonjoom opened this Issue Dec 4, 2017 · 22 comments

Comments

Projects
None yet
5 participants
@simonjoom

simonjoom commented Dec 4, 2017

plugin-proposal-decorators create for each component using decorator same function template:

applyDecoratedDescriptor
and
initDefineProp

so the code ended is much more bigger,
i propose to create only this once in global what do you think?
(It s what i did inside babelHelper ...)

same this plugins use the function ensureInitializerWarning who create no useful bit for each component too:
function NAME(descriptor, context){ throw new Error( 'Decorating class property failed. Please ensure that proposal-class-properties is enabled and set to use loose mode. To use proposal-class-properties in spec mode with decorators, wait for he next major version of decorators in stage 2

will be better to add it in global too ...

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Dec 4, 2017

Collaborator

Hey @simonjoom! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

Collaborator

babel-bot commented Dec 4, 2017

Hey @simonjoom! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@xtuc xtuc added the Needs Info label Dec 4, 2017

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Dec 4, 2017

Collaborator

Hi @simonjoom! A maintainer of the project has notified me that you're missing
some information we'll need to replicate this issue.

Please understand that we receive a high volume of issues, and there are only a limited number
of volunteers that help maintain this project. The easier it is for us to decipher an issue with the info provided,
the more likely it is that we'll be able to help.

Please make sure you have the following information documented in this ticket:

  1. Your Babel configuration (typically in the form of a .babelrc)
  2. The current (incorrect) behavior you're seeing
  3. The behavior you expect
  4. A short, self-contained example

Please provide either a link to the problem via the repl, or if the repl is insufficient,
a new and minimal repository with instructions on how to build/replicate the issue.

Collaborator

babel-bot commented Dec 4, 2017

Hi @simonjoom! A maintainer of the project has notified me that you're missing
some information we'll need to replicate this issue.

Please understand that we receive a high volume of issues, and there are only a limited number
of volunteers that help maintain this project. The easier it is for us to decipher an issue with the info provided,
the more likely it is that we'll be able to help.

Please make sure you have the following information documented in this ticket:

  1. Your Babel configuration (typically in the form of a .babelrc)
  2. The current (incorrect) behavior you're seeing
  3. The behavior you expect
  4. A short, self-contained example

Please provide either a link to the problem via the repl, or if the repl is insufficient,
a new and minimal repository with instructions on how to build/replicate the issue.

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Dec 4, 2017

Member

Could you please use our ISSUE_TEMPLATE?

Member

xtuc commented Dec 4, 2017

Could you please use our ISSUE_TEMPLATE?

@simonjoom

This comment has been minimized.

Show comment
Hide comment
@simonjoom

simonjoom Dec 4, 2017

It's not really an issue, but an improvement to reduce bundle code result.

The things are very simple to understand with what i explained.

simonjoom commented Dec 4, 2017

It's not really an issue, but an improvement to reduce bundle code result.

The things are very simple to understand with what i explained.

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Dec 4, 2017

Member

This is an issue to me because we could do something better than we currently do.

In the template there is an input code and your possible proposition, this would help us better understand your issue.

Member

xtuc commented Dec 4, 2017

This is an issue to me because we could do something better than we currently do.

In the template there is an input code and your possible proposition, this would help us better understand your issue.

@simonjoom

This comment has been minimized.

Show comment
Hide comment
@simonjoom

simonjoom Dec 6, 2017

I give more info:
i used the last code (4 days ago imported) of babel 7

here my babel config

"babelproduction": {
    "presets": [
      [
        "es2015",
        {
          "modules": false,
          "loose": true
        }
      ],
      "react",
      "flow"
    ],
    "plugins": [
      "babel-plugin-root-import",
      "babel-plugin-syntax-dynamic-import",
      "babel-plugin-transform-async-to-generator",
      "babel-plugin-proposal-object-rest-spread",
      "babel-plugin-proposal-optional-catch-binding",
      "babel-plugin-proposal-unicode-property-regex",
      "babel-plugin-transform-exponentiation-operator",
      "babel-plugin-proposal-decorators",
      [
        "proposal-class-properties",
        {
          "loose": true
        }
      ],
      "proposal-export-namespace-from",
      "proposal-export-default-from",
      "babel-plugin-external-helpers",
      "babel-plugin-dual-import"
    ]

simonjoom commented Dec 6, 2017

I give more info:
i used the last code (4 days ago imported) of babel 7

here my babel config

"babelproduction": {
    "presets": [
      [
        "es2015",
        {
          "modules": false,
          "loose": true
        }
      ],
      "react",
      "flow"
    ],
    "plugins": [
      "babel-plugin-root-import",
      "babel-plugin-syntax-dynamic-import",
      "babel-plugin-transform-async-to-generator",
      "babel-plugin-proposal-object-rest-spread",
      "babel-plugin-proposal-optional-catch-binding",
      "babel-plugin-proposal-unicode-property-regex",
      "babel-plugin-transform-exponentiation-operator",
      "babel-plugin-proposal-decorators",
      [
        "proposal-class-properties",
        {
          "loose": true
        }
      ],
      "proposal-export-namespace-from",
      "proposal-export-default-from",
      "babel-plugin-external-helpers",
      "babel-plugin-dual-import"
    ]
@simonjoom

This comment has been minimized.

Show comment
Hide comment
@simonjoom

simonjoom Dec 6, 2017

if i do something like
@Inject("appstate") @Observer
class Component1 extends Component {
//blabla blab
}

@Inject("appstate") @Observer
class Component2 extends Component {
//blabla blab
}

Babel insert
applyDecoratedDescriptor template
initDefineProp template
and the ensureInitializerWarning template in each component.

So it will be better to add only one time on global (as the function of babelHelpers do)

hope it will help more

simonjoom commented Dec 6, 2017

if i do something like
@Inject("appstate") @Observer
class Component1 extends Component {
//blabla blab
}

@Inject("appstate") @Observer
class Component2 extends Component {
//blabla blab
}

Babel insert
applyDecoratedDescriptor template
initDefineProp template
and the ensureInitializerWarning template in each component.

So it will be better to add only one time on global (as the function of babelHelpers do)

hope it will help more

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 6, 2017

Member

The correct fix for this bug is moving the decorators helpers (

const buildInitializerWarningHelper = template(`
function NAME(descriptor, context){
throw new Error(
'Decorating class property failed. Please ensure that ' +
'proposal-class-properties is enabled and set to use loose mode. ' +
'To use proposal-class-properties in spec mode with decorators, wait for ' +
'the next major version of decorators in stage 2.'
);
}
`);
const buildInitializerDefineProperty = template(`
function NAME(target, property, descriptor, context){
if (!descriptor) return;
Object.defineProperty(target, property, {
enumerable: descriptor.enumerable,
configurable: descriptor.configurable,
writable: descriptor.writable,
value: descriptor.initializer ? descriptor.initializer.call(context) : void 0,
});
}
`);
const buildApplyDecoratedDescriptor = template(`
function NAME(target, property, decorators, descriptor, context){
var desc = {};
Object['ke' + 'ys'](descriptor).forEach(function(key){
desc[key] = descriptor[key];
});
desc.enumerable = !!desc.enumerable;
desc.configurable = !!desc.configurable;
if ('value' in desc || desc.initializer){
desc.writable = true;
}
desc = decorators.slice().reverse().reduce(function(desc, decorator){
return decorator(target, property, desc) || desc;
}, desc);
if (context && desc.initializer !== void 0){
desc.value = desc.initializer ? desc.initializer.call(context) : void 0;
desc.initializer = undefined;
}
if (desc.initializer === void 0){
// This is a hack to avoid this being processed by 'transform-runtime'.
// See issue #9.
Object['define' + 'Property'](target, property, desc);
desc = null;
}
return desc;
}
`);
) to the babel-helpers package.

Member

nicolo-ribaudo commented Dec 6, 2017

The correct fix for this bug is moving the decorators helpers (

const buildInitializerWarningHelper = template(`
function NAME(descriptor, context){
throw new Error(
'Decorating class property failed. Please ensure that ' +
'proposal-class-properties is enabled and set to use loose mode. ' +
'To use proposal-class-properties in spec mode with decorators, wait for ' +
'the next major version of decorators in stage 2.'
);
}
`);
const buildInitializerDefineProperty = template(`
function NAME(target, property, descriptor, context){
if (!descriptor) return;
Object.defineProperty(target, property, {
enumerable: descriptor.enumerable,
configurable: descriptor.configurable,
writable: descriptor.writable,
value: descriptor.initializer ? descriptor.initializer.call(context) : void 0,
});
}
`);
const buildApplyDecoratedDescriptor = template(`
function NAME(target, property, decorators, descriptor, context){
var desc = {};
Object['ke' + 'ys'](descriptor).forEach(function(key){
desc[key] = descriptor[key];
});
desc.enumerable = !!desc.enumerable;
desc.configurable = !!desc.configurable;
if ('value' in desc || desc.initializer){
desc.writable = true;
}
desc = decorators.slice().reverse().reduce(function(desc, decorator){
return decorator(target, property, desc) || desc;
}, desc);
if (context && desc.initializer !== void 0){
desc.value = desc.initializer ? desc.initializer.call(context) : void 0;
desc.initializer = undefined;
}
if (desc.initializer === void 0){
// This is a hack to avoid this being processed by 'transform-runtime'.
// See issue #9.
Object['define' + 'Property'](target, property, desc);
desc = null;
}
return desc;
}
`);
) to the babel-helpers package.

@simonjoom

This comment has been minimized.

Show comment
Hide comment
@simonjoom

simonjoom Dec 6, 2017

yes i guess

simonjoom commented Dec 6, 2017

yes i guess

@perinikhil

This comment has been minimized.

Show comment
Hide comment
@perinikhil

perinikhil Dec 11, 2017

Contributor

Hi guys,
I would like to make a contribution here. Could someone help me with what i'm supposed to do to resolve this issue?

Contributor

perinikhil commented Dec 11, 2017

Hi guys,
I would like to make a contribution here. Could someone help me with what i'm supposed to do to resolve this issue?

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Dec 11, 2017

Member

@perinikhil as far as I understand, since the "runtime" for decorator is in the plugin it will be injected for each run. We need to move them into our Babel helpers because we can dedupe them.

Maybe @nicolo-ribaudo can tell you more about that.

Member

xtuc commented Dec 11, 2017

@perinikhil as far as I understand, since the "runtime" for decorator is in the plugin it will be injected for each run. We need to move them into our Babel helpers because we can dedupe them.

Maybe @nicolo-ribaudo can tell you more about that.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 11, 2017

Member

@perinikhil As you can see in the code included in my previous comment, in the decorators plugin are defined some "helpers".

Babel inserts this kind of helpers for many features, for example classes (As you can see in the repl, it injects the _classCallCheck and _instanceof functions).
The helpers injected for decorators look similar (repl), but there is a difference: the helpers used by the classes plugin can be "removed" using babel-plugin-transform-runtime or babel-plugin-external-helpers. It works because they aren't defined directly in the plugin, but in the babel-helpers package.
By "removing" the hepers, we avoid rewriting them in every file.

To make these two plugins correctly work with babel-plugin-proposal-decorators, you need to remove the helpers from the plugin itself and add then to babel-helpers/src/helpers.js.

If you need any help, ping me on slack (https://babeljs.slack.com)

Member

nicolo-ribaudo commented Dec 11, 2017

@perinikhil As you can see in the code included in my previous comment, in the decorators plugin are defined some "helpers".

Babel inserts this kind of helpers for many features, for example classes (As you can see in the repl, it injects the _classCallCheck and _instanceof functions).
The helpers injected for decorators look similar (repl), but there is a difference: the helpers used by the classes plugin can be "removed" using babel-plugin-transform-runtime or babel-plugin-external-helpers. It works because they aren't defined directly in the plugin, but in the babel-helpers package.
By "removing" the hepers, we avoid rewriting them in every file.

To make these two plugins correctly work with babel-plugin-proposal-decorators, you need to remove the helpers from the plugin itself and add then to babel-helpers/src/helpers.js.

If you need any help, ping me on slack (https://babeljs.slack.com)

@simonjoom

This comment has been minimized.

Show comment
Hide comment
@simonjoom

simonjoom Dec 11, 2017

Me i resolved this and move these helper by hand in global in my code ..
waiting for the correct update with babelhelpers

simonjoom commented Dec 11, 2017

Me i resolved this and move these helper by hand in global in my code ..
waiting for the correct update with babelhelpers

@perinikhil

This comment has been minimized.

Show comment
Hide comment
@perinikhil

perinikhil Dec 12, 2017

Contributor

So i'm guessing this issue has already been claimed? :(
Anything else i could do as my first PR then guys? :D

Contributor

perinikhil commented Dec 12, 2017

So i'm guessing this issue has already been claimed? :(
Anything else i could do as my first PR then guys? :D

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 12, 2017

Member

@perinikhil I think that @simonjoom only worked around this issue by manually editing Babel's output

Member

nicolo-ribaudo commented Dec 12, 2017

@perinikhil I think that @simonjoom only worked around this issue by manually editing Babel's output

@simonjoom

This comment has been minimized.

Show comment
Hide comment
@simonjoom

simonjoom Dec 12, 2017

yes it is what i did is hacky

simonjoom commented Dec 12, 2017

yes it is what i did is hacky

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 12, 2017

Member

@perinikhil Then you can work on this ☺️

Member

nicolo-ribaudo commented Dec 12, 2017

@perinikhil Then you can work on this ☺️

@perinikhil

This comment has been minimized.

Show comment
Hide comment
@perinikhil

perinikhil Dec 12, 2017

Contributor

@nicolo-ribaudo
can you please add me to the babeljs slack channel?
My email id is - hidden

Contributor

perinikhil commented Dec 12, 2017

@nicolo-ribaudo
can you please add me to the babeljs slack channel?
My email id is - hidden

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc
Member

xtuc commented Dec 12, 2017

@perinikhil

This comment has been minimized.

Show comment
Hide comment
@perinikhil

perinikhil Dec 12, 2017

Contributor

@xtuc I need some help while moving the functions to babel-helpers/src/helpers.js as they cannot contain any placeholders...simply moving them won't work 😮

Contributor

perinikhil commented Dec 12, 2017

@xtuc I need some help while moving the functions to babel-helpers/src/helpers.js as they cannot contain any placeholders...simply moving them won't work 😮

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 12, 2017

Member

Since the only placeholder used is the function name, you can hardcode it (like is is done for the other helpers)

Member

nicolo-ribaudo commented Dec 12, 2017

Since the only placeholder used is the function name, you can hardcode it (like is is done for the other helpers)

@simonjoom

This comment has been minimized.

Show comment
Hide comment
@simonjoom

simonjoom Dec 15, 2017

i tried to compile the last babel 7 code, this update break the compilation.
There is something to test ...

simonjoom commented Dec 15, 2017

i tried to compile the last babel 7 code, this update break the compilation.
There is something to test ...

@lock lock bot added the outdated label May 3, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.