Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

chore(npm): Make require()-able as part of publish script#10731

Closed

Conversation

bclinkinbeard
Copy link
Contributor

Deletes main field of package.json and creates index.js that is CommonJS compatible

/cc @caitp

@caitp
Copy link
Contributor

@petebacondarwin I'd like to get this into the release today. Just going to test it real quick. That cool with you?

@caitp
Copy link
Contributor

so, on my system, the sed call on package.json is getting -e passed as the value to -i, which is weird. I'm not sure if that's happening for you or not, but we might need to add a safety check just to prevent that from happening

@bclinkinbeard
Copy link
ContributorAuthor

Not sure I follow. I'm not super familiar with sed tbh, but the deleteJsonProp function is using sed -i -e, which could very well be wrong. Is that the call you are referring to?

@caitp
Copy link
Contributor

yes, I thought you added it, didn't check the diff =) @IgorMinar do you know if we were using that utility before and it worked right on jenkins?

@caitp
Copy link
Contributor

wait, it is added in the diff :< hmmm

@bclinkinbeard
Copy link
ContributorAuthor

Oh, I should probably just remove -e, no?

@bclinkinbeard
Copy link
ContributorAuthor

Just pushed another commit combining the flags. I think that is right...

@caitp
Copy link
Contributor

no, you definitely don't want to combine the flags... here's an idea, just leave it as it was before and delete the package.json-e if it exists, that probably is fine

@bclinkinbeard
Copy link
ContributorAuthor

delete the package.json-e if it exists

Sorry, I don't know what you mean by that.

@caitp
Copy link
Contributor

.. if the file exists (package.json-e), remove it

@caitp
Copy link
Contributor

well, I guess this won't make it for 1.3.9 :( sorry man.

@caitpcaitp added this to the 1.3.x milestone Jan 12, 2015
@caitp
Copy link
Contributor

...or maybe it will, we'll see :>

@bclinkinbeard
Copy link
ContributorAuthor

I think this should be correct. Apparently some versions of sed require a suffix be passed to -i, so I've set it to use a blank string.

Fingers crossed! :)

@petebacondarwin
Copy link
Contributor

sed is well outside of my comfort zone. If you two are happy with that then so am I.

@caitp
Copy link
Contributor

just tested with the latest commit, looks good here

@caitp
Copy link
Contributor

lgtm~

@caitp
Copy link
Contributor

eh pete, should we backport this? I guess future 1.3 releases would want it too

@bclinkinbeard
Copy link
ContributorAuthor

Sweet!

@petebacondarwin
Copy link
Contributor

@caitp - let's put it in 1.4 and 1.3 but not 1.2 OK?
Feel free to merge then we have our release SHAs.

caitp pushed a commit that referenced this pull request Jan 13, 2015
(This has not been tested locally with browserify --- but it should work! If it doesn't, please file a bug rather than just leaving a comment on this commit :) Closes#10731
@caitp
Copy link
Contributor

it is done~

@caitpcaitp closed this in babc20bJan 13, 2015
@spacepluk
Copy link

thank you guys :)

@petebacondarwin
Copy link
Contributor

I understand that there might be a problem with this?

@bclinkinbeard
Copy link
ContributorAuthor

@petebacondarwin Yea we realized some shortcomings. #10732 now has the correct, improved version.

spacepluk pushed a commit to spacepluk/angular.js that referenced this pull request Jan 19, 2015
(This has not been tested locally with browserify --- but it should work! If it doesn't, please file a bug rather than just leaving a comment on this commit :) Closesangular#10731
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.
close