-
Notifications
You must be signed in to change notification settings - Fork 318
pubish umd to npm mk II #479
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
Conversation
.travis.yml
Outdated
| secure: fEP9K7y0ZF9fRvQEUN4kgPXQEZvi3Cx3ikUebG2UM/2uhcaUQm0+KpgZ2S+lvOTYcBnNgzPzFsVIZMcVcTxwIKuQDEMq9y2eop2hnisb9KXsIm9qPYSzOnRm74VuiOt4hNOZMe0qVBK2cO3vC9NDXuzdI8A0JynJhthfl4t+kFM= | ||
| - provider: npm | ||
| email: [email protected] | ||
| api_key: YOUR_SECURE_API_KEY_HERE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codingjoe you'll need to add your npm credentials here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure you need to duplicate the on: section for both providers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a key when I merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure
codingjoe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good I did have a couple questions, but the feature itself looks fine. Good that we talked about it :)
.travis.yml
Outdated
| secure: fEP9K7y0ZF9fRvQEUN4kgPXQEZvi3Cx3ikUebG2UM/2uhcaUQm0+KpgZ2S+lvOTYcBnNgzPzFsVIZMcVcTxwIKuQDEMq9y2eop2hnisb9KXsIm9qPYSzOnRm74VuiOt4hNOZMe0qVBK2cO3vC9NDXuzdI8A0JynJhthfl4t+kFM= | ||
| - provider: npm | ||
| email: [email protected] | ||
| api_key: YOUR_SECURE_API_KEY_HERE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure you need to duplicate the on: section for both providers.
.travis.yml
Outdated
| secure: fEP9K7y0ZF9fRvQEUN4kgPXQEZvi3Cx3ikUebG2UM/2uhcaUQm0+KpgZ2S+lvOTYcBnNgzPzFsVIZMcVcTxwIKuQDEMq9y2eop2hnisb9KXsIm9qPYSzOnRm74VuiOt4hNOZMe0qVBK2cO3vC9NDXuzdI8A0JynJhthfl4t+kFM= | ||
| - provider: npm | ||
| email: [email protected] | ||
| api_key: YOUR_SECURE_API_KEY_HERE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a key when I merge.
| @@ -1,4 +1,15 @@ | |||
| (function ($) { | |||
| /* global define, jQuery */ | |||
| (function (factory) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not so firm with modern day JavaScript. Why do we need all this boilerplate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's one for amd, one for commonjs and one for browser globals. It's known as UMD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You gotta give me more. Consider me a JS noob. Is that really needed or just a nice to have?
I really want to keep this simple... I guess you know why ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's so it works with webpack, require.js and as a django static asset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment where the wrapper snippet is coming from. That way future contributors will know whats up.
| @@ -0,0 +1,11 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file omited in the setuptools package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, I'll double check when I merge ;)
| import os | ||
|
|
||
| def main(): | ||
| with open('package.json', 'r+') as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should open this writable, should you not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what 'r+' means I'm told
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry. You need read/write, makes sense.
|
@codingjoe any update on this? |
|
@graingert did you give me access yet for the npm package? |
|
@codingjoe what's your npm username, again? |
|
@graingert |
|
@codingjoe done |
|
@codingjoe any update on this? |
|
Travis is currently not working. I will tag this once it's working again and check if it releases correctly. If I forget please ping me again ;) |
|
@codingjoe awesome thanks |
|
Ah your commit actually broke the travis config. I missed that. I should be working now. I am just waiting for the deploy to succeed. |
|
Released in 6.3.0 |
|
ok @graingert, I think you will need to put a bit more love into this |
|
Looks like you need to 'skip_cleanup' to deploy build artifacts:
Cleaning up git repository with `git stash --all`. If you need build
artifacts for deployment, set `deploy.skip_cleanup: true`. See
https://docs.travis-ci.com/user/deployment#Uploading-Files-and-skip_cleanup.
…On Wed, 22 Aug 2018, 13:01 Johannes Hoppe, ***@***.***> wrote:
ok @graingert <https://github.com/graingert>, I think you will need to
put a bit more love into this
https://travis-ci.org/applegrew/django-select2/jobs/419135968
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#479 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAZQTEJXXvcKxwNxHLDR6QwNyF652aTIks5uTUgWgaJpZM4UETxO>
.
|
|
@graingert I can't wait for your pull-request 👍 |
This doesn't include the manual media changes