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

stampit({options}) api #84

Merged
merged 9 commits into from
May 31, 2015
Merged

stampit({options}) api #84

merged 9 commits into from
May 31, 2015

Conversation

koresar
Copy link
Member

@koresar koresar commented May 30, 2015

Please, review. List of changes:

  • New stampit( options ) API.
  • Convenience shortcut methods, like stampit.methods(), etc. See Convenience methods stampit.state(), stampit.methods(), stampit.enclose() #41.
  • Reorganized tests a bit for more convenience.
  • Drop IE8 support.
  • /dist/ is now not committed to the repo but build only on the prepublish NPM hook.
  • Excluded /test/, travis, jshist, SauceLabs, etc. development files from NPM package via .npmignore.

stampit.methods, stampit.refs, stampit.init, stampit.props. Drop IE8 support. Reorganize tests, add some.
Exclude tests, travis, jshist, SauceLabs, etc. development files from NPM package.
@ericelliott
Copy link
Contributor

  • /dist/ is now not committed to the repo but build only on the prepublish NPM hook.

Do you have a link to a reference where I can learn more about this? Is the latest /dist/ build still available when people run npm install in their apps? IMO, it's a little weird that it won't be browsable in the repository on GitHub...

  • Excluded /test/, travis, jshist, SauceLabs, etc. development files from NPM package via .npmignore.

Probably a good idea.

* @param {Object} [refs] A map of property names and values to be mixed into each new object.
* @param {Function} [init] A closure (function) used to create private data and privileged methods.
* @param {Object} [props] An object to be deeply cloned into each newly stamped object.
* @param {Object} [options] Options to build stamp from: `{ methods, refs, init, props }`
Copy link
Contributor

Choose a reason for hiding this comment

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

There should still be @param annotations for each of:

options.methods, options.refs, options.init, and options.props.

I wonder if it's possible to define an options type with JSDoc without it assuming that there's an options class?

Then you could define the type elsewhere and just do @param {options} [options] desc...

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a way. I'll see what I can do.

@troutowicz
Copy link
Member

@ericelliott

Do you have a link to a reference where I can learn more about this? Is the latest /dist/ build still available when people run npm install in their apps?

https://docs.npmjs.com/misc/scripts

First bullet point. There's really no need to have dist/ in the repo, noone looks at it. It's just for compatibility. I do it the way @koresar describes for react-stampit.

@koresar We should use the files key of package.json instead of .npmignore.
https://docs.npmjs.com/files/package.json#files

files: {
  stampit.js,
  mixer.js,
  ... other files to include
}

require('./stampit-aliases');
require('./state');
require('./v1-compatibility');
require('require-all')(__dirname);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ericelliott
Copy link
Contributor

@troutowicz Thanks for the reference. I was unaware of this part:

"(Also run on local npm install without any arguments.)"

I think that answers my question, so the dist build of stampit will be available if we add it as "main", right?

I ask because I'd love to convert Stampit source to ES6 and compile it, but we'll still need to support require in ES5 projects. =)

@troutowicz
Copy link
Member

Exactly.

"main": "dist/stampit.js"

I can help out with the ES6 conversion. I recently submitted a PR to @koresar's supermixer repo doing just that. I want to see all stampit projects using ES6!

@ericelliott
Copy link
Contributor

I want to see all stampit projects using ES6!

👍

Let's do that right after we land 2.0. =)

@ericelliott
Copy link
Contributor

My comments are just minor nitpicks. I'm OK with landing this.

  1. Are there other PRs we need to merge first?
  2. Are the rest of the to-do items for 2.0 ready?

Please feel free to merge any pending PRs necessary to land 2.0. I'm excited to share all your great contributions with the world. =)

@koresar
Copy link
Member Author

koresar commented May 31, 2015

@ericelliott

  1. There are no outstanding PR's to my knowledge.
  2. I'm trying to keep the outstanding things in the Milestone 2.0. The only thing left is README.

RE the prepublish and dist/. Each time you'll execute npm publish the dist/ will be generated and uploaded to the NPM registry.

@koresar
Copy link
Member Author

koresar commented May 31, 2015

@troutowicz I do not feel strong about files feature in package.json. I'll leave .npmignore for the time being. We'll see how it goes.

@koresar
Copy link
Member Author

koresar commented May 31, 2015

Updated the JSDoc of stampit() to describe argument. JSDoc docs: http://usejsdoc.org/tags-param.html

@koresar
Copy link
Member Author

koresar commented May 31, 2015

#74 got implemented as well (static stuff). Requires thorough review. @ericelliott ?

@ericelliott
Copy link
Contributor

The io.js install is failing.

Looks like Travis can't find lodash 3.9.1+ even though it's currently at 3.9.3. Am I reading that right?

@koresar
Copy link
Member Author

koresar commented May 31, 2015

Correct. I have no idea why is that happening. I tried to use earlier lodash, but it fails with "Can't download package from the HTTPS uri". Might be an NPM issue (too old build).

@ericelliott
Copy link
Contributor

This looks good to me. I'm happy to merge it but maybe we should hold off on npm publish until we sort out why the travis build is failing?

ericelliott pushed a commit that referenced this pull request May 31, 2015
@ericelliott ericelliott merged commit cd74356 into v2_0 May 31, 2015
@koresar
Copy link
Member Author

koresar commented May 31, 2015

You can fixing it. :) Just put the following into the travis yaml.

before_install
  npm update npm -g

@ericelliott
Copy link
Contributor

Oh, cool! Can you do that in a new PR? This one is already merged. =)

@koresar
Copy link
Member Author

koresar commented Jun 2, 2015

Btw, Travis-CI fixed their io.js+lodash bug. Builds are now green. :)

@ericelliott
Copy link
Contributor

Nice.

@koresar koresar deleted the options-API branch June 4, 2015 12:40
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