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.

Sign up for GitHub

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

Jump to bottom

Fixes 6965 #7017

Merged
merged 2 commits into from
Dec 13, 2017
Merged

Fixes 6965 #7017

merged 2 commits into from
Dec 13, 2017

Conversation

perinikhil
Copy link
Contributor

@perinikhil perinikhil commented Dec 12, 2017 •
edited
Loading

Q                       A
Fixed Issues? Fixes #6965
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

nicolo-ribaudo
nicolo-ribaudo reviewed Dec 12, 2017
View reviewed changes
packages/babel-plugin-proposal-decorators/src/index.js Outdated
const helper = buildApplyDecoratedDescriptor({
NAME: state.applyDecoratedDescriptor,
});
const helper = state.addHelper("buildApplyDecoratedDescriptor");
path.scope.getProgramParent().path.unshiftContainer("body", helper);
Copy link
Member

Choose a reason for hiding this comment

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

The problem is that these lines:

const helper = buildApplyDecoratedDescriptor({
  NAME: state.applyDecoratedDescriptor,
});
path.scope.getProgramParent().path.unshiftContainer("body", helper);

added the helper function to the file.

On the other hand, state.addHelper("buildApplyDecoratedDescriptor"); returns the id of the function (not the function itself), and the helper is added automatically.

So you should just remove the ensure* functions (which are the equivalent of state.addHelper), and replace calls to them (like

babel/packages/babel-plugin-proposal-decorators/src/index.js

Line 273 in f5ef928

ensureApplyDecoratedDescriptorHelper(path, state),
) with addHelper(" ... ").

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 see, yes makes sense!
I will update this PR soon after some more digging and tinkering 😃

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 13, 2017 •
edited
Loading

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6174/

@perinikhil perinikhil changed the title WIP: Fixes 6965 Fixes 6965 Dec 13, 2017
@nicolo-ribaudo nicolo-ribaudo added the PR: Polish 💅 A type of pull request used for our changelog categories label Dec 13, 2017
xtuc
xtuc approved these changes Dec 13, 2017
View reviewed changes
Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

Thanks @perinikhil that looks good to me

@perinikhil
Copy link
Contributor Author

Thank you @xtuc and @nicolo-ribaudo for your valuable inputs and patience! Much obliged 😃
I've also raised a docs PR which could be reviewed as well...
And also, i'd like to pick up another issue, any suggestions?

@xtuc
Copy link
Member

xtuc commented Dec 13, 2017

Feel free to merge @nicolo-ribaudo if it looks good to you.

@perinikhil, either:

nicolo-ribaudo
nicolo-ribaudo approved these changes Dec 13, 2017
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thank you! 🎉

@nicolo-ribaudo nicolo-ribaudo merged commit 9cc0a26 into babel:master Dec 13, 2017
TrejGun
TrejGun reviewed Dec 15, 2017
View reviewed changes
packages/babel-helpers/src/helpers.js
helpers.applyDecoratedDescriptor = defineHelper(`
export default function _applyDecoratedDescriptor(target, property, decorators, descriptor, context){
var desc = {};
Object['ke' + 'ys'](descriptor).forEach(function(key){
Copy link

Choose a reason for hiding this comment

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

what is the problem to use Object.keys(descriptor) ?

Copy link
Member

Choose a reason for hiding this comment

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

It is kept because it was in the original helper, I will check

Copy link
Member

Choose a reason for hiding this comment

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

Because of transform-runtime, same here https://github.com/babel/babel/pull/7017/files/805c8f7cf2dc6eea9d3dcb85b55acc6a6a3334e5#diff-ba75bd4d116cc90ad8c8002e3010e821R762

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Dec 15, 2017
Merged
@blikblum blikblum mentioned this pull request Apr 23, 2018
Closed
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TrejGun TrejGun TrejGun left review comments

@xtuc xtuc xtuc approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

reduce code generated of plugin-proposal-decorators
5 participants
@perinikhil @babel-bot @xtuc @TrejGun @nicolo-ribaudo