Skip to content

Decorator#10

Open
pconerly wants to merge 1 commit intojasonslyvia:masterfrom
pconerly:decorator
Open

Decorator#10
pconerly wants to merge 1 commit intojasonslyvia:masterfrom
pconerly:decorator

Conversation

@pconerly
Copy link
Copy Markdown

@pconerly pconerly commented May 19, 2016

@jasonslyvia

This is for issue #7

A few changes that should have a discerning eye:

  • Moved some of the functionality to src/core.js
  • Since this is a decorator and not a handler, I couldn't figure out a good way keep the activate.call(this, rowIdentifier, handler);. I think the user should bind their handler themselves. I'll call that out in the documentation.
  • I haven't worked on many node packages--- will importing require('react-menu-aim/decorator') work with this setup?

Left to do:

  • Needs LGTM from @jasonslyvia
  • Figure out the / importing
  • Needs documentation in the README

@jasonslyvia
Copy link
Copy Markdown
Owner

require('react-menu-aim/decorator') will work only if the module consumer transform our source into ES5, ideally we should do this job, so maybe put decorator.js in src/ and compile it using babel then put it ./decorator.js

@jasonslyvia
Copy link
Copy Markdown
Owner

I'll look into this PR later but generally I think it's the right way to go, kudos!

Comment thread .babelrc
@@ -0,0 +1 @@
{ "stage": 1 } No newline at end of file
Copy link
Copy Markdown

@gabrielbull gabrielbull May 19, 2016

Choose a reason for hiding this comment

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

Why use Babel 5? Here's a config for Babel 6:

{
  "presets": [
    "es2015",
    "stage-1",
    "react"
  ],
  "plugins": [
    "transform-decorators-legacy"
  ]
}

Shouldn't we also have babel in the devDependencies:

    "babel-core": "^6.9.0",
    "babel-plugin-transform-decorators-legacy": "^1.3.4",
    "babel-preset-es2015": "^6.9.0",
    "babel-preset-react": "^6.5.0",
    "babel-preset-stage-1": "^6.5.0",

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Whoops, you are right. I was using babel6, I think that babel6 is just backwards-compatible?

I was enabling stage:1 to turn on decorators.

Copy link
Copy Markdown

@gabrielbull gabrielbull May 19, 2016

Choose a reason for hiding this comment

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

Umm, Babel 6 is not backwards compatible, you must have Babel 5 running somehow (?). Decorators have been removed from stage 1 and are now in the transform-decorators-legacy plugin. So I guess this could be the config:

{
  "presets": [
    "es2015",
    "react"
  ],
  "plugins": [
    "transform-decorators-legacy"
  ]
}
    "babel-core": "^6.9.0",
    "babel-plugin-transform-decorators-legacy": "^1.3.4",
    "babel-preset-es2015": "^6.9.0",
    "babel-preset-react": "^6.5.0",

Copy link
Copy Markdown
Author

@pconerly pconerly May 19, 2016

Choose a reason for hiding this comment

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

Sorry, I was confused. The repo is using babelify version 6.1.2, which uses babel-core version 5: https://github.com/babel/babelify/blob/v6.1.2/package.json

Anyway, upgrading to babel6 is something that we should do, but it's out-of-scope for this PR.

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.

babelify is only intended for demos that using ES6 syntax, the module itself (index.js) is written in pure ES5 so no transformation is needed.

But I guess it's been a long time since I released this repo initially, the tool chains changed a lot, maybe it worth upgrading to state of the art.

@sankhadeeproy007
Copy link
Copy Markdown

Any ideas on how to use this?

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.

4 participants