Skip to content
This repository has been archived by the owner on Jun 18, 2019. It is now read-only.

Updates to support UMD default export & maintain es6 for lib in npm p… #80

Merged
merged 1 commit into from May 2, 2016
Merged

Conversation

patrick-rodgers
Copy link
Contributor

…ackage

@pbjorklund
Copy link
Contributor

This feels extremely hacky and I think we should really think about wether this is a good idea or if we are just bending backwards to do something that's not used by any other lib compiled from ts.

@patrick-rodgers
Copy link
Contributor Author

patrick-rodgers commented May 3, 2016

As I said in the other thread. It is 100% valid to replace the exports with an object, in our case PnP so you can include it directly. We've been doing this from the very beginning. The issue is that there is a limitation with using the "export =" syntax which supports this in TypeScript and enabling the es6 style import syntax instead of the "older" require() syntax. Based on the TS docs there is no way to support both natively, which is why I added this. I don't want to make folks do a var blah = new Core(). That's an extra step for no reason, pushing the limitation in TS compilation down to the consumer.

If anyone has better ideas around how to handle this I am willing to discuss.

@pbjorklund
Copy link
Contributor

No need to be annoyed @patrick-rodgers, we're all working towards the same goal here.

Doing string replacement of the transpiled source with gulp is what I consider "extremely hacky"

.pipe(replace(/Object\.defineProperty\(exports, "__esModule", \{ value: true \}\);/ig, "")) .pipe(replace(/exports.default = PnP;/ig, "return PnP;"))

I wouldn't consider it unreasonable to think about this?

If you read the other thread (#78) I'm not opposed to consumption of the lib being done via pnp.sp.blah. The new Core() thing was an idea that I wanted to get out there before v1. Never meant to get anyone upset over it.

@pbjorklund
Copy link
Contributor

pbjorklund commented May 3, 2016

If we add

export const sharepoint = new SharePoint();
export const sp = new Rest();

to pnp.ts alongside

export default class PnP {
    public static sharepoint = new SharePoint();
    public static sp = new Rest();
}

we get

image

Perhaps that's a way to keep the pnp.sp usage even when browseringfying the transpiled es6?

/lib/pnp.ts output is (truncated)

export const sharepoint = new SharePoint();
export const sp = new Rest();
export default class PnP {
}
/**
 * Utility methods
 */
PnP.util = Util;
/**
 * The full SharePoint library
 */
PnP.sharepoint = new SharePoint();

Thoughts @patrick-rodgers @mike-morrison ?

Edit: I don't envision exposing the object like this, just stuck both in there to show what happened.
Edit2: It requires us not to use import * as Util from "./utils/Util";, could be solved with #86

@patrick-rodgers
Copy link
Contributor Author

bottling up the util functions is fine with me, it was like that at the beginning and was changed to the current syntax.

The export const stuff feels like we are duplicating things. Using the replaces during package is so far the best thing I can come up with. We are just stuck with a limitation of TypeScript. I am happy to leave this for now unless we find a big problem we need to solve around this.

@pbjorklund
Copy link
Contributor

Not sure what you mean with duplicating things.To be clear I was talking about removing the class and export the objects as consts, not having both.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants