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

fix: move Jest to dev dependencies, close #89 #91

Closed
wants to merge 3 commits into from

Conversation

bahmutov
Copy link

@bahmutov bahmutov commented Dec 16, 2019

Note: this throws this error, which does not affect the install

Running sanity check... 
Could not update list of known top-level domains for parse-domain because of "Cannot find module '.bin/jest'
Require stack:
- /private/tmp/node_modules/parse-domain/scripts/build-tries.js"

@bahmutov bahmutov changed the title move Jest to dev dependencies, close #89 fix: move Jest to dev dependencies, close #89 Dec 16, 2019
@flotwig
Copy link

flotwig commented Dec 17, 2019

@bahmutov won't this make the step to build the fresh public suffix list always fail? I have no earthly clue why it requires jest, but it does, or it will throw here and won't complete:

childProcess.execFileSync(process.execPath, [require.resolve(".bin/jest")], {
cwd: rootPath,
encoding: "utf8",
});

@flotwig
Copy link

flotwig commented Dec 17, 2019

The line above it says "Running sanity check", so I'm guessing the purpose is to run all the Jest specs using the new data before finalizing it. I guess that makes sense to do but.... it also seems likely that the relevant specs could be extracted, re-written to use basic assertions, and then it wouldn't need the prod dependency on jest.

@bahmutov
Copy link
Author

yes, it does include Jest just to run a basic test, which is not good in production mode

@flotwig
Copy link

flotwig commented Dec 17, 2019

Not to mention the jest test seems to fail: #90 I can also repro locally.

@jhnns
Copy link
Member

jhnns commented Jan 12, 2020

Yes, Jest is a regular dependency. Jest is not used at runtime but on every npm install. However, some people had problems with this approach (#42) so we will revert it back to manual updates.

@jhnns jhnns closed this Jan 12, 2020
@schmod
Copy link

schmod commented Jan 23, 2020

@jhnns Anything we can do to help?

@jhnns
Copy link
Member

jhnns commented Jan 27, 2020

Nope, thanks. It's almost finished :)

@jhnns jhnns mentioned this pull request Jan 30, 2020
@jhnns jhnns mentioned this pull request Mar 9, 2020
jhnns added a commit that referenced this pull request Apr 23, 2020
…ements

- Run against [psl example domains](https://raw.githubusercontent.com/publicsuffix/list/master/tests/test_psl.txt). Closes #1
- Add support for international domain names. Fixes #16 #82 and #44
- Only accept hostnames instead of whole URLs. Fixes #49 and #14
- Do not auto update tries on npm install. Fixes #42 #48 and #90
- Use "node-fetch" instead of "got". Fixes #78 and #62
- Use Node's "assert" module instead of Jest for smoke test. Fixes #92 #93 #89 #91
- Recognize IPv4 and IPv6 in hostnames. Fixes #102

BREAKING CHANGE: This release is a complete rewrite in TypeScript. It fixes some long outstanding bugs and comes with improvements we were planning for quite some time. The major changes are: 1. parseDomain does not accept whole URLs anymore. Only the hostname section of a URL is allowed now. 2. We removed the options object. Custom TLDs are returned as "valid but not listed". The parse result contains both the result with private TLDs and without private TLDs. 3. Dropped Node 6 support. We recommend reading the README since the public API as changed quite a lot.
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.

Remove jest from dependencies
4 participants