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

WIP for Meteor integration #27

Closed
wants to merge 1 commit into from

Conversation

chip
Copy link
Contributor

@chip chip commented May 1, 2015

@someatoms - Since I'm new to this process, this might take a few iterations before it will be ready. I'd suggest having @splendido take a look and give the thumbs up before merging.

There are a few TODO's that we'll need to fill in as well.

@splendido - I used a sample package.js and package.json that @dandv created for the FontAwesome project, so hopefully that will be a good starting point for this package. Please LMK your thoughts and if there's an easy way for me to test this integration.

Thanks!

@chip
Copy link
Contributor Author

chip commented May 1, 2015

This PR is for #19.

@splendido
Copy link

Hi @chip and @someatoms,

From what I can see, the only thing we have to sort out is how to keep version numbers inside package.json, bower.json and package.js in sync.
This somehow depends on the build process @someatoms uses to create new releases.
Since there's no tarce of an automated build process, I guess the version number was bumped manually so far.

Would it be fine to have to bump them manually also in the future?
Would this be error prone?

@simonbengtsson
Copy link
Owner

I have used git tags for versions as bower doesn't actually use the version property right now as far as I understand. Or is the bower version property required in some way?

With the package.js implementation in this PR the only place the version is required to be manually entered is in the packages.json which is fine with me. I could probably just add a git hook which automatically updates it to match the latest git tag (could also be done for bower I suppose).

@chip
Copy link
Contributor Author

chip commented May 1, 2015

@someatoms - I only put the version in bower.json because I saw it used in another example for Meteor Packaging. Upon a second review, I see that I should've put that in the package.json file instead of bower.json. If that sounds reasonable, what version would you prefer?

@simonbengtsson
Copy link
Owner

Sounds good! The current version of the plugin is 1.2.3.

@chip chip force-pushed the meteor-integration branch 2 times, most recently from f832963 to 01d72ed Compare May 1, 2015 16:33
@chip
Copy link
Contributor Author

chip commented May 1, 2015

Great! I just pushed up those revisions.

Also, please see the TODO in package.js:

// TODO This package need to be published on npmjs.org

It will only be used as a fallback, so there's probably no rush, but thought you should know.

@splendido
Copy link

Hey guys,
if the version bumping is not a problem (and maybe could be automatized), I'd prefer to keep the package.js as lean as the following:

// Package metadata file for Meteor.js.
'use strict';

Package.describe({
  name: 'jspdf:autotable',  // http://atmospherejs.com/jspdf/autotable
  summary: 'PDF table generator in javascript (jspdf plugin)',
  version: '1.2.3',
  git: 'https://github.com/someatoms/jsPDF-AutoTable.git',
  documentation: 'README.md'
});

Package.onUse(function (api) {
  api.imply('jspdf:jspdf');
  api.addFiles('jsPDF-AutoTable/jspdf.plugin.autotable.js', 'client');
});

@chip: we should finish the publish for jspfd:jspdf first which needs to be implied by this package...

In any case, I'd also like to package to be kept up to date with autopublish.meteor.com not to ask @someatoms to manually publish for meteor on his own.

See the PR we had for bootstrap with some more details about how it works.

@simonbengtsson
Copy link
Owner

Just found out that jspdf does not work with node so I won't publish this library on npm either. With that and autopublish in mind the package.js splendido posted seems to be everything needed for official meteor support?

@chip
Copy link
Contributor Author

chip commented May 14, 2015

@someatoms - Thanks for the info on jspdf. Earlier this week @splendido and I completed the Meteor integration for jspdf:core. I still need to test it locally, but once that is done I'll use that as a boilerplate for your package. If you have time, feel free to check it out.

@chip
Copy link
Contributor Author

chip commented May 16, 2015

@splendido - I think the issue is that https://github.com/MeteorPackaging/jspdf-autotable is a fork of the this repo, but we've recently decided to approach this in a different manner. I think the MeteorPackaging repo just needs to be a blank slate instead of a fork. Then I can issue my PR against that and we can merge it in there instead of here.

Once that is done, we can have @someatoms sign up at autopublish and test.

Your thoughts?

@chip
Copy link
Contributor Author

chip commented May 16, 2015

In case it's not clear, my previous comment is in regards to this PR: https://github.com/someatoms/jsPDF-AutoTable/pull/31.

@splendido
Copy link

I've just created jspdf-AutoTable-wrapper.
This way the only thing we'll need from @someatoms will be a webhook to les us know about new releases and this repo could remain clean without additional files for Meteor only cluttering it.

@simonbengtsson
Copy link
Owner

Cool! Webhook to where?

@chip
Copy link
Contributor Author

chip commented May 16, 2015

@someatoms - Check this out - https://autopublish.meteor.com/howitworks. There's a brief video and a few steps to read through there. Basically, once this webhook is setup it will autopublish your changes to Atmosphere.

@simonbengtsson
Copy link
Owner

I don't think that page explain to what url, or what kind of webhook that should be used. In the video the webhook is automatically added, but since this repo is no meteor package I don't think it is possible? I assume https://autopublish.meteor.com/publish and webhook only for push events however?

@splendido
Copy link

Hey @someatoms, sorry for the fragmented info!

Yes the video is for enabling repositories containing Meteor packages.
This time we're following a bit different direction though...

Coould you please set up a webhook for us looking at this example?
And yes, the address should now be https://autopublish.meteor.com/publish with https' and nothttp`.

Thanks a bunch!

@splendido
Copy link

Wow, that was quick!

Webhook received :-)

@simonbengtsson
Copy link
Owner

Sweet! I get a warning, but if it works its all good I suppose. screenshot_2015-05-17-11-03-40

@splendido
Copy link

mmm, weird!
Could you please have a look among the details of the failed delivery and check the response tab?
It might be I'm sending out an invalid response, but at least the hook arrived...

@splendido
Copy link

No more need to check!
It should have got "No subscription found for this repository!" as reply error, but that's somehow correct since manual hook creation for non-meteor package repos is still not supported.

But the hook is there and we have its details in the DB now! :-)

I'd say this concludes this integration process.

Thanks again for your availability and collaboration!!!

@simonbengtsson
Copy link
Owner

I know many people who are using meteor, but have not had the opportunity myself to check it out in any bigger project yet. So it's great that you guys made this happen! Thanks!

@splendido
Copy link

@someatoms, I've just noticed you've used tags and not releases for your latest versions...
...could you please update the webhook to reflect these settings: basically we'd like to be informed when you create a new tag ;-)

...as the very last request: would it be too much asking you to keep also a version field inside the bower.json file like MrRio is doing for jsPDF here? This way we're going to be able to read it directly from the bower.json file instead of using the tag name...

Thanks again,
Luca

@simonbengtsson
Copy link
Owner

Before the webhook was triggered only when new tags were created. So it should already have been correct right? Anyway I updated the webhook to trigger when new releases are published as well just in case I do that in the future. Version number in the bower.json file is probably a good idea. Will probably do that in the next version. All good!

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.

3 participants