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

bundles it for the browser #113

Merged
merged 4 commits into from
Feb 2, 2018
Merged

Conversation

srashid5
Copy link
Contributor

@srashid5 srashid5 commented Nov 17, 2017

this adds bundling for the browser
see #37, #89

package.json Outdated
"posttest": "npm run lint && npm run test-browser",
"prepublish": "npm run update",
"test": "nyc mocha -R dot -r env-test",
"test-browser": "testling",
"test-watch": "mocha -R dot -r env-test --watch",
"update": "node ./bin/update.js",
"version": "npm run generate-changelog && git add CHANGELOG.md"
"version": "npm run generate-changelog && git add CHANGELOG.md",
"build": "browserify index.js --s tldjs > tld.js",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it also run npm run update to fetch the latest version of publicsuffix list before bundling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea running npm run update before bundling makes total sense, thanks for pointing out

what do you think about bundling in a postinstall script? i feel that can be omitted

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should omit it from postinstall? I feel like keeping commands simple and predictable is a good thing, let's not do too much at once.

Copy link
Collaborator

@remusao remusao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@srashid5
Copy link
Contributor Author

thanks @remusao,
any ideas why the build is failing? It runs okay locally

@remusao
Copy link
Collaborator

remusao commented Nov 17, 2017

It happened before, and it seems to only fail for node v6, while running tests in the browser with teslting. That's weird, I will try to investigate.

@thom4parisot
Copy link
Owner

Hm, I'm not sure to understand the goal of this pull request, considering the code can already by bundled in any project using browserify/npm.

Also, is there a specific lifecycle moment when npm run build and npm run build-min should be triggered?

@srashid5
Copy link
Contributor Author

srashid5 commented Nov 23, 2017

hey @oncletom, I've been using this package for one of the projects i'm working on and I ran into this situation where I needed a bundled JS file on client-side as well, but I had to bundle it myself as you mentioned, also I saw others wanting something similar.

So I just thought it would be more consistent if the package had bundled JS files as well.

I originally thought that bundling should happen post installation, but changed it later.

Feel free to close this PR if you think otherwise.

@thom4parisot
Copy link
Owner

Let's go ahead.

It might be of the interest of cdnjs/cdnjs#8943 :-)

@thom4parisot thom4parisot merged commit 2663273 into thom4parisot:master Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants