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

Rename docker examples to TypeScript #342

Closed
wants to merge 3 commits into from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Apr 9, 2020

Wanted to add an interface for #337 but noticed the docker examples are .js not .ts files.

There's one ts-ignore added (process.on("unhandledRejection")), because the typing doesn't match. I preferred this rather than commenting out the block.

@glensc
Copy link
Contributor Author

glensc commented Apr 9, 2020

@evantahler could you enlighten why examples use relative paths in git repo?

import { Worker, Scheduler } from "../../../src";
/* In your projects:
import { Scheduler, Worker } from "node-resque";
*/

why can't this be flipped over? so that most users when they copy the files, don't have to modify them to get them to work.

for example, had to make uncommitted changes to docker examples to test that the containers work properly.

@evantahler
Copy link
Member

I believe that it is generally poor form to 'run' typescript in production, and that the recommended approach is to compile it first and then run the generated javascript files. That's why these docker examples only attempt to run the JS code which would be pre-compiled... which comes from the node-resque NPM package. We build the javascript and that's the main export of the published package.

That said, you are totally correct and the import statements should just use node-resque normally (not relative paths). In these examples we should be using the node-resque package already published to NPM, not the local version.

References:

@evantahler
Copy link
Member

I'll take a pass at updating these today

@evantahler
Copy link
Member

closing this in favor of #343

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.

2 participants