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

caitp commented Jan 12, 2015

@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

caitp commented Jan 12, 2015

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
Contributor Author

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

caitp commented Jan 12, 2015

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

caitp commented Jan 12, 2015

wait, it is added in the diff :< hmmm

@bclinkinbeard
Copy link
Contributor Author

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

@bclinkinbeard
Copy link
Contributor Author

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

@caitp
Copy link
Contributor

caitp commented Jan 12, 2015

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
Contributor Author

delete the package.json-e if it exists

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

@caitp
Copy link
Contributor

caitp commented Jan 12, 2015

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

@caitp
Copy link
Contributor

caitp commented Jan 12, 2015

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

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

caitp commented Jan 12, 2015

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

@bclinkinbeard
Copy link
Contributor Author

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

caitp commented Jan 13, 2015

just tested with the latest commit, looks good here

@caitp
Copy link
Contributor

caitp commented Jan 13, 2015

lgtm~

@caitp
Copy link
Contributor

caitp commented Jan 13, 2015

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

@bclinkinbeard
Copy link
Contributor Author

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

caitp commented Jan 13, 2015

it is done~

@caitp caitp closed this in babc20b Jan 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
Contributor Author

@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 :)

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

Successfully merging this pull request may close these issues.

5 participants