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

[email protected] #471

Merged

Conversation

digigarlab
Copy link

Hi there,

I'm using [email protected] with levelup and redis as a db.
I'm not using leveldown/levelDB so I don't want to install it (plus, it doesn't build on the specific node-alpine version I am targeting), that's why I am installing search-index with the --no-optional option.

The problem is, despite leveldown being marked as optional, it's required at the top of the search-index/index.js, so when I start my app it immediately crashes, complaining about leveldown not being resolvable.

The fix is easy: move the require down, so that it's only executed when necessary.

/!\ This shouldn't be merged to master

As I am using an older version of search-index (and I can't easily upgrade) this should be merged in a separate release branch. I saw you have tags for versions, but no release branches.
Could you create a release branche for version 0.15? It can done that way:

git checkout v0.15.1 # checkout the tag, in a detached state
git checkout -b release/0.15

Then push this fix as v0.15.2?

Cheers

@eklem eklem requested a review from fergiemcdowall March 29, 2019 12:44
@fergiemcdowall
Copy link
Owner

Change looks good, I just need to find out how organised I was when I pushed out [email protected] (can we get the source code at that point in time)

@fergiemcdowall
Copy link
Owner

Hi @digigarlab

I'm afraid that we have been a bit disorganised in the last year or so, and we have just been pushing out releases from master without making branches or tags (we actually used to do this before, and should start to do so again).

That said, your pull request is on the latest code- if you want to we can merge it into the latest version?

(And we will make a tagged release this time I promise! :) )

F

@digigarlab
Copy link
Author

Hi @fergiemcdowall

I'm afraid that we have been a bit disorganised in the last year or so, and we have just been pushing out releases from master without making branches or tags

There is actually a tag for v0.15.1. I based my commit on this one.

That said, your pull request is on the latest code

Yes, because there is no release branch associated to this code, but as I said previously it's trivial to do so, you can just run:

git checkout v0.15.1 # checkout the tag, in a detached state
git checkout -b release/0.15 # Or any other name you want for your release branch
git push # Push the release branch to the main repo

If you do so I will rebase this PR on to this release branch.

if you want to we can merge it into the latest version?

The code changed too much since v0.15.1 I'm afraid.

@fergiemcdowall
Copy link
Owner

Ah- thanks for pointing that out. I have created a new branch called release/0.15 - could you rebase the PR? Once that is done I will push out a "new" release of 0.15.x to NPM

@digigarlab digigarlab changed the base branch from master to release/0.15 March 29, 2019 14:08
@digigarlab
Copy link
Author

I just did it, thanks!

@fergiemcdowall fergiemcdowall merged commit 5653eac into fergiemcdowall:release/0.15 Mar 29, 2019
@digigarlab digigarlab deleted the fix-leveldown-import branch March 29, 2019 14:11
@digigarlab digigarlab restored the fix-leveldown-import branch March 29, 2019 14:18
@fergiemcdowall
Copy link
Owner

fergiemcdowall commented Mar 29, 2019

Just published [email protected] to npm.

@digigarlab does it work as intended?

@eklem
Copy link
Collaborator

eklem commented Mar 29, 2019

@fergiemcdowall Think you need to publish a v1.0.6 so that 0.15.2 isn't taken as the latest when npm install search-index

@fergiemcdowall
Copy link
Owner

Ahh- on it!

@fergiemcdowall
Copy link
Owner

I just ran npm dist-tag add [email protected] latest

so

fergus ~/tmp 💥  ➜ npm view search-index

[email protected] | MIT | deps: 5 | versions: 114
A network resilient, persistent full-text search library for the browser and Node.js
https://github.com/fergiemcdowall/search-index#readme

keywords: index, language, lucene, natural, offline, search

dist
.tarball: https://registry.npmjs.org/search-index/-/search-index-1.0.5.tgz
.shasum: 8e0700887778c7658aee453522b922534a355e54
.integrity: sha512-7coSJELhXztPUW9/F7Sc0iw3EyXnvr7CUgnUMczv23xbmqo8uV4ea2bopS9SPeuXSym8KKFqyR6rOhBNLI7EuA==
.unpackedSize: 842.1 kB

dependencies:
@babel/polyfill: ^7.2.5        fergies-inverted-index: 0.0.12 http-serve: ^1.0.1             term-vector: ^1.0.0            traverse: ^0.6.6

maintainers:
- eklem <[email protected]>
- fergie <[email protected]>
- mewwts <[email protected]>

dist-tags:
latest: 1.0.5

published a week ago by eklem <[email protected]>

But my npm install is still fetching 0.15.2. I guess this is a caching issue? Will try again in a few minutes

@fergiemcdowall
Copy link
Owner

Must be caching- its working now:

fergus ~/tmp 💥  ➜ npm install search-index

> [email protected] install /home/fergus/tmp/node_modules/leveldown
> node-gyp-build


> [email protected] postinstall /home/fergus/tmp/node_modules/level
> opencollective-postinstall || exit 0

Thank you for using level!
If you rely on this package, please consider supporting our open collective:
> https://opencollective.com/level/donate

npm WARN saveError ENOENT: no such file or directory, open '/home/fergus/tmp/package.json'
npm WARN enoent ENOENT: no such file or directory, open '/home/fergus/tmp/package.json'
npm WARN tmp No description
npm WARN tmp No repository field.
npm WARN tmp No README data
npm WARN tmp No license field.

+ [email protected]
added 70 packages from 63 contributors and audited 89 packages in 6.437s
found 0 vulnerabilities

@eklem
Copy link
Collaborator

eklem commented Mar 29, 2019

I got the latest now.

@eklem
Copy link
Collaborator

eklem commented Mar 29, 2019

I guess NPM have gotten a little dependent on caching =)

Screenshot 2019-03-29 at 15 57 39

@fergiemcdowall
Copy link
Owner

Hehe- yes, I'm not complaining :)

@digigarlab digigarlab deleted the fix-leveldown-import branch March 29, 2019 16:42
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.

3 participants