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

Can't create multiple presentations in node #83

Closed
daynedavis opened this issue May 11, 2017 · 13 comments
Closed

Can't create multiple presentations in node #83

daynedavis opened this issue May 11, 2017 · 13 comments
Assignees
Milestone

Comments

@daynedavis
Copy link

I am requiring pptx via var pptx = require('pptxgenjs') within a function called by a route on my node server, but when I hit the endpoint multiple times slides are added to the same presentation (first time it returns a ppt with 4 slides, second time one with 8 slides). Changing module.exports to be PptxGenJs in pptxgen.js and then requiring pptx = new (require('pptxgenjs'))() allowed me to get separate instances. Is this a bug or is there a way to get multiple instances in node without making that change?

@gitbrent gitbrent self-assigned this May 15, 2017
@gerbendekker
Copy link

That would be nice, having the same issue here.
Needed to restart the electron app for each new pptx.
What I did is creating a new public function 'newFile' but don't know this is appropriate.

this.newFile = function newFile() {
    gObjPptx = {};
    gObjPptx.author = 'PptxGenJS';
    gObjPptx.company = 'PptxGenJS';
    gObjPptx.revision = '1';
    gObjPptx.subject = 'PptxGenJS Presentation';
    gObjPptx.title = 'PptxGenJS Presentation';
    gObjPptx.fileName = 'Presentation';
    gObjPptx.fileExtn = '.pptx';
    gObjPptx.pptLayout = LAYOUTS['LAYOUT_16x9'];
    gObjPptx.slides = [];
}

gitbrent pushed a commit that referenced this issue May 17, 2017
@gitbrent
Copy link
Owner

Hi @daynedavis @gerbendekker ,

Thanks for the question.

I've added a new section to the README that covers this very issue:
Saving a Node.js Presentation

See also: Issue #38

@gerbendekker
Copy link

How does this cover the NodeJs usecase of above?
Can't 'new' the PptxGenJS class because it isn't exposed.

var pptx = require("pptxgenjs");

@daynedavis
Copy link
Author

@gitbrent like @gerbendekker said I'm not able use new in NodeJs since the class is not exposed.

@gitbrent
Copy link
Owner

Hi @daynedavis @gerbendekker,

Ack! I spaced out and used the wrong code. Sorry about that.

It should've been pptx = require("pptxgenjs"); and i've corrected the README as well.

Does this work for you guys, or would a more formal method like clearPresentation() (something that would remove all the Slides from the current Presentation) be preferable?

@gerbendekker
Copy link

gerbendekker commented May 26, 2017

Now we are back where we started.
Let me write some more context about the issue in NodeJs:

Problem

let pptx = require('pptxgenjs');
let slide1 = pptx.addNewSlide();
let slide2 = pptx.addNewSlide();

pptx = require('pptxgenjs');
// How many slides does pptx have?

The answer is 2, because require('pptxgenjs') returns the same (singleton) instance.

Solution
What I would expect in 'pptxgen.js' and described by @daynedavis:

module.exports = PptxGenJS;

Then we can write:

let PptxGenJS = require('pptxgenjs');
let pptx = new PptxGenJS();
let slide1 = pptx.addNewSlide();
let slide2 = pptx.addNewSlide();

pptx = new PptxGenJS();
// Yeay a new instance.

But that is a breaking change.
For the development roadmap of version 2.0 I would advise to export the class instead of an instance.

Thinking forward...
What use case would clearPresentation() have in 2.0? I would use new PptxGenJS().
But I can imagine a use case for removeSlide().
My advise would be adding a removeSlide(pageNumber) to version 1 and we can iterate to clear the presentation.

@gitbrent gitbrent added this to the 2.0.0 milestone May 26, 2017
@LiranBri
Copy link

LiranBri commented Aug 8, 2017

I fixed it in the current version by deleting the module from the cache before requiring again:

  delete require.cache[require.resolve('pptxgenjs')]
  let pptx = require('pptxgenjs')

@labriola
Copy link

Here is a much cleaner work around:

var pptxgenjs = require('pptxgenjs');
var pptx = new pptxgenjs.constructor();
...
pptx.save(name);

const pptx2 = new pptxgenjs.constructor();
...
pptx2.save(name);

Using the constructor property of the function will let you build new objects at will.

@dtabolich
Copy link

Any news guys?

@gitbrent
Copy link
Owner

Hi @dtabolich,

Other than the newer posts above, no. The factory method will be updated in 2.0 as it's a breaking change, until then work-arounds are all that's available. :-(

@labriola
Copy link

labriola commented Nov 22, 2017

@gitbrent let me know your thoughts on this. It's not ideal but it would solve the issue in a way that node users should expect and keep things compatible.

labriola pushed a commit to DigitalPrimates/PptxGenJS that referenced this issue Nov 22, 2017
…n additional property is exposed for use with the new operator
@gitbrent
Copy link
Owner

gitbrent commented Nov 24, 2017

Hi All,

UPDATE: A fix for this issue is now checked into the 2.0.0 codebase!

Individual, singleton instances of PptxGenJS are now created using the standard Node syntax:

var PptxGenJS = require("pptxgenjs");
let pptx = new PptxGenJS();

If any of you would care to demo it and provide feedback, i'd appreciate it. :-)

New Node Examples:

  • The examples/node-demo.js file now creates 2 separate Presentations to demo the new functionality.

See README section for more:

@gitbrent
Copy link
Owner

Version 2.0.0 has been released and this issue is now fixed.

Thank you all for your feedback.

ericrockey pushed a commit to WeTransfer/PptxGenJS that referenced this issue Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants