#17: Replace resource.js with ouroboros#18
Conversation
There was a problem hiding this comment.
Thanks @kostiukevych-ls !
This looks good from a business logic perspective, but there's a missing piece: @applura/ouroboros needs to be bundled into the build artifact so that the client will work in users' projects via our CDN.
When I run npm run build I'm warned about unresolved dependencies:
Following the link to https://rollupjs.org/troubleshooting/#warning-treating-module-as-external-dependency suggests that we need to add https://github.com/rollup/plugins/tree/master/packages/node-resolve
package.json
Outdated
| "build": "rollup index.js --file dist/v2.js", | ||
| "check": "npm run fmt -- --check && npm run lint && npm test", | ||
| "test": "deno test --allow-net", | ||
| "test": "deno install && deno test --allow-net", |
There was a problem hiding this comment.
Why do we need this? Could it be addressed by updating the GitHub workflow?
There was a problem hiding this comment.
Make sense. Thank you for comment. Updated
|
@gabesullice Thank you for your comments. I'm not entirely sure if I implemented everything correctly—could you please take a look? Let me know if there's anything else I should update or change. |
rollup.config.cjs
Outdated
| const { nodeResolve } = require("@rollup/plugin-node-resolve"); | ||
|
|
||
| module.exports = { | ||
| input: "src/cli.js", |
There was a problem hiding this comment.
I haven't tried to run the build, but this config can't be right because this file doesn't exist.
Can you run the build and verify it works? If it does work, I think it means we don't need this input configuration at all and should remove it so it doesn't confuse us later.
gabesullice
left a comment
gabesullice
left a comment
There was a problem hiding this comment.
Thanks @kostiukevych-ls! There's a couple bits of dead code, probably from debugging. This works as expected though, so I'll just push a commit to clean them up.
rollup.config.cjs
Outdated
| const { nodeResolve } = require("@rollup/plugin-node-resolve"); | ||
|
|
||
| module.exports = { | ||
| input: "index.js", |
There was a problem hiding this comment.
| input: "index.js", |
I don't think we need this line.
| output: { | ||
| file: "dist/v2.js", | ||
| }, | ||
| plugins: [nodeResolve()], |
package.json
Outdated
| "rollup": "^3.8.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@rollup/plugin-commonjs": "^28.0.6", |
There was a problem hiding this comment.
| "@rollup/plugin-commonjs": "^28.0.6", |
Not needed.
2 participants
Closes #17