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

feat: allow using streetmerchant as a library #2038

Closed
wants to merge 5 commits into from

Conversation

gmalette
Copy link
Contributor

Description

As explained in #2034, I'm trying to use streetmerchant as a library in another project that exposes a different dashboard.
To allow this, I need to make a few changes to this codebase. This PR contains 3 changes:

  1. It adds a version number to package.json, as npm requires a version to be present.
  2. It loads the version.txt file from a path relative to banner.ts.
  3. It bumps the support to node <15.11.0. This is not necessary.

Testing

Ran the program, still works

@gmalette gmalette requested a review from jef as a code owner February 26, 2021 20:24
@@ -1,5 +1,6 @@
{
"name": "streetmerchant",
"version": "3.4.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a version number here will increase the maintenance burden because it requires updating versions in two places. I can change banner.ts to use this number if that's preferable.

The changes in this file aren't strictly necessary as it's easy to make them myself and then tell git to make them assume-unchanged.

Copy link
Owner

Choose a reason for hiding this comment

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

Before we make this change, I'd like to update the pipeline to manage this for us. This is easy enough, but just needs an update.

@@ -17,7 +18,7 @@
"test:notification:production": "node build/test/functional/test-notification.js"
},
"engines": {
"node": ">=12.0.0 <15.0.0"
"node": ">=12.0.0 <15.11.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that there could be good reasons for keeping <15.0.0, which I don't know.

Copy link
Owner

Choose a reason for hiding this comment

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

I was keep it below 15 only because some folks were having problems with tsc. We should probably get on latest sooner than later though!


const version = readFileSync('version.txt', 'utf8');
const version = readFileSync(path.join(__dirname, '../../version.txt'), 'utf8');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would really like to get this change in if possible. I'm not terribly familiar with the typescript compiler nor the node runner, but when I run it from my other project, the version.txt file isn't found as it's looking for it somewhere else.

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting... Yeah I'm getting the version running as is, but if you're running within Docker, this could be an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was looking in the path of the project that wraps it.

@jef
Copy link
Owner

jef commented Mar 2, 2021

Alright, made a PR (#2066)

This should get the things you need!

@jef jef closed this Mar 2, 2021
@gmalette
Copy link
Contributor Author

gmalette commented Mar 2, 2021

Alright! Thanks a bunch!

@gmalette gmalette deleted the gm/packaging branch March 2, 2021 21:38
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

2 participants