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.

Sign up for GitHub

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

Jump to bottom

feat!: accept database instance instead of path when creating server #93

Merged
merged 4 commits into from Feb 1, 2023
Merged

feat!: accept database instance instead of path when creating server #93

merged 4 commits into from Feb 1, 2023

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Jan 30, 2023

Closes #92

This is a breaking change, as it updates the function signature of the export createServer function to accept an instance of a BetterSqlite3 database instead of a file path string.

  • Removes better-sqlite3 from depedencies field
  • Adds better-sqlite3 to peerDependencies and devDependencies fields
  • Updates MapServerOptions from { dbPath: string } to { database: Database }

gmaclennan
gmaclennan requested changes Jan 31, 2023
View reviewed changes
package.json Show resolved Hide resolved
src/api/index.ts Outdated
) => {
if (dbPath == null)
throw new Error('Map server option `dbPath` must be specified')
if (database == null || !(database instanceof Db))
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be unreliable as a check. Because it is now a peer dependency, it is possible that an app using this code could create the instance with a different version of better-sqlite3 than the one that resolves here. Also requiring it in this file makes it not really a peer dependency (because this will break if it is not installed). I think we need to lean on typescript for checking, and it we really want to check then maybe check a property or method? However if it is not a BetterSqlite3.Database instance then something will throw later on pretty quick too.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yes true! totally overlooked that. will remove the instanceOf check but not sure i can think of a better check, so maybe okay to omit and let things break in that case

Copy link
Member Author

Choose a reason for hiding this comment

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

Also requiring it in this file makes it not really a peer dependency (because this will break if it is not installed)

does this mean that I can't import the TS type from it either? guessing the answer is yes

Copy link
Member Author

Choose a reason for hiding this comment

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

ah looks like this may be what i'm looking for: https://stackoverflow.com/questions/54392809/how-do-i-handle-optional-peer-dependencies-when-publishing-a-typescript-package/70096366#70096366

seems to work as expected when trying locally via npm pack

@gmaclennan
Copy link
Member

gmaclennan commented Jan 31, 2023 via email

achou11
achou11 commented Jan 31, 2023
View reviewed changes
src/api/index.ts
Comment on lines +1 to +2
// @ts-ignore
import type { Database } from 'better-sqlite3'
Copy link
Member Author

Choose a reason for hiding this comment

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

we only want to work with the type, see #93 (comment)

@achou11 achou11 merged commit 17d5fa8 into master Feb 1, 2023
@achou11 achou11 deleted the 92/better-sqlite3-peer-dep branch February 1, 2023 14:19
EvanHahn added a commit that referenced this pull request Feb 11, 2024
@EvanHahn
feat!: initialize with storage path instead of database
d59fe99 
The core of this change:

```diff
 const mapServer = createMapServer(
   {},
-  { database: new Database('./example.db') }
+  { storagePath: './map-server-example.db' }
 )
```

See also: [#92], [#93], [this comment][comment].

[#92]: #92
[#93]: #93
[comment]: #100 (comment)
@EvanHahn EvanHahn mentioned this pull request Feb 11, 2024
Merged
EvanHahn added a commit that referenced this pull request Feb 12, 2024
@EvanHahn
feat!: initialize with storage path instead of database
0b2467f 
The core of this change:

```diff
 const mapServer = createMapServer(
   {},
-  { database: new Database('./example.db') }
+  { storagePath: './map-server-example.db' }
 )
```

See also: [#92], [#93], [this comment][comment].

[#92]: #92
[#93]: #93
[comment]: #100 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmaclennan gmaclennan gmaclennan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

better-sqlite3 as a peer dep
2 participants
@achou11 @gmaclennan