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

init #1

Merged
merged 32 commits into from
Dec 19, 2022
Merged

init #1

merged 32 commits into from
Dec 19, 2022

Conversation

ivibumblebee
Copy link
Contributor

No description provided.

.github/workflows/deploy-core.yml Outdated Show resolved Hide resolved
Comment on lines +33 to +35
# - name: Run unit tests
# working-directory: ./core
# run: yarn test
Copy link
Member

Choose a reason for hiding this comment

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

Should this be uncommented or is this because there currently are not tests??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there aren't currently tests, next dev day perhaps

core/README.md Outdated
**Middleware**

```ts
export const i18NextPDFGenerator = pdfMiddlewareBuilder(
Copy link
Member

Choose a reason for hiding this comment

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

I would maybe call i18NextPDFGenerator i18NextPDFMiddleware to make it clear that this returns an express compatible middleware.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm good point

"homepage": "https://github.com/AES-Outreach/express-react-pdf#readme",
"dependencies": {
"@react-pdf/renderer": "^3.0.0",
"express": "^4.18.1",
Copy link
Member

Choose a reason for hiding this comment

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

Regarding express potentially not needing to be a dependency, we could take a look at other packages that supply express compatible middleware and see if they have express as a dependency in any way. I wouldn't use a single example to make the decision but if we can find a few packages that might give us an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So looking at this: microsoft/types-publisher#81
I think they are supposed to be in dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But honestly i'm still not totally sure. Most of the express middleware packages I found are not typescript and don't seem to have it in dependencies

@ivibumblebee ivibumblebee merged commit 8c45a74 into master Dec 19, 2022
@ivibumblebee ivibumblebee deleted the f/init branch December 19, 2022 15:39
Copy link

@Woodpile37 Woodpile37 left a comment

Choose a reason for hiding this comment

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

i was redirected to accept changes

@ivibumblebee
Copy link
Contributor Author

i was redirected to accept changes

could you please clarify?

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