[WIP] feat: expressjs framework #784
Conversation
yanksyoon
force-pushed
the
feat/expressjs-framework
branch
from
056fbc1 to
8afa87e
Compare
There was a problem hiding this comment.
Hi, I took a brief look at what's in the tutorial so far, and I left two comments.
I have some other comments that I excluded because they will affect all of the tutorials. I'd rather keep the set of tutorials consistent for now and update them all in one go.
| :end-before: [docs:curl-expressjs-bare-rock-end] | ||
| :dedent: 2 | ||
|
|
||
| Unsurprisingly, the ExpressJS application should still respond with |
There was a problem hiding this comment.
@erinecon do you think we should have the "Unsuprisingly" qualifier here? Maybe some users are suprised that we got rid of a bunch of stuff and it is still working
There was a problem hiding this comment.
// [docs:append-lines]
var timeRouter = require('./routes/time');
app.use('/time', timeRouter);
// [docs:append-lines-end]
export app;There was a problem hiding this comment.
And then use the start-after, end-before directive
| parts: | ||
| expressjs-framework/install-app: | ||
| npm-include-node: false |
There was a problem hiding this comment.
Perhaps remove this since it is the default
There was a problem hiding this comment.
And perhaps move this as the first one
| parts: | ||
| expressjs-framework/install-app: | ||
| npm-include-node: false |
There was a problem hiding this comment.
We can remove this and have it before the other bare one
| @@ -131,7 +131,7 @@ execute: | | |||
| retry -n 5 --wait 2 curl --fail localhost:8000/time/ | |||
| # [docs:curl-time] | |||
| curl localhost:8000/time/ | |||
| curl --fail localhost:8000/time/ | |||
There was a problem hiding this comment.
Could we do this for all extensions and all curl commands please
There was a problem hiding this comment.
Can we remove this? It should get auto generated
| @@ -117,7 +117,7 @@ execute: | | |||
| retry -n 5 --wait 2 curl --fail localhost:8000/time | |||
|
|
|||
| # [docs:curl-time] | |||
| curl localhost:8000/time | |||
| curl --fail localhost:8000/time | |||
There was a problem hiding this comment.
Also for the other curls for this file
| @@ -118,7 +118,7 @@ execute: | | |||
| retry -n 5 --wait 2 curl --fail localhost:8000/time | |||
| # [docs:curl-time] | |||
| curl localhost:8000/time | |||
| curl --fail localhost:8000/time | |||
There was a problem hiding this comment.
Also here for the other curls
| @override | ||
| def get_parts_snippet(self) -> dict[str, Any]: | ||
| """Return the parts to add to parts. | ||
|
|
||
| This is unused but is required by the ABC. | ||
| """ | ||
| return {} |
There was a problem hiding this comment.
Perhaps switch the order to match the other frameworks
| logpath_report=False, | ||
| ) | ||
| package_json_contents = package_json_file.read_text(encoding="utf-8") | ||
| return json.loads(package_json_contents) |
There was a problem hiding this comment.
Let's check whether it is a dictionary since it could be something else. We should probably also check for invalid JSON
| doc_slug="/reference/extensions/expressjs-framework", | ||
| logpath_report=False, | ||
| ) | ||
| if "name" not in self._app_package_json: |
There was a problem hiding this comment.
Let's check name is a string
| "cp ${CRAFT_PART_BUILD}/.npmrc ${CRAFT_PART_INSTALL}/lib/node_modules/" | ||
| f"{self._app_name}/.npmrc\n" | ||
| f"ln -s /lib/node_modules/{self._app_name} ${{CRAFT_PART_INSTALL}}/app\n" |
There was a problem hiding this comment.
Could we document why we do this in the explanation please?
| ) | ||
| return install_app_part | ||
|
|
||
| @property |
There was a problem hiding this comment.
Check whether these can be functions
There was a problem hiding this comment.
let's remove all of these
| docker run --name "${NAME}-container" -d -p 8137:3000 "${IMAGE}" | ||
| retry -n 5 --wait 2 curl localhost:8137 | ||
| http_status=$(curl -s -o /dev/null -w "%{http_code}" localhost:8137) | ||
| [ "${http_status}" -eq 200 ] |
There was a problem hiding this comment.
Perhaps extend this to also customise the version of node
There was a problem hiding this comment.
let's check we get the expected version of node
| @pytest.mark.parametrize( | ||
| "package_json_contents, error_message", | ||
| [ | ||
| ("{}", "missing start script"), |
There was a problem hiding this comment.
Perhaps include not a dict and a name not a string
|
|
||
|
|
||
| @pytest.mark.usefixtures("expressjs_extension", "package_json_file") | ||
| def test_expressjs_extension_default( |
There was a problem hiding this comment.
let's put the 4 scenarios here:
- 24.04 base and node not customised
- 24.04 base and node customised
- bare base and node not customised
- bare base and node version customised
Then we don't need the sub functions. It would probably be best to do this using parametrize
Adds support for ExpressJS framework.
Key Decisions
bareand24.04only. Drop support for 22.04 (jammy)??operator commonly used in JS projects. This means that the projects initialized withexpress-generatortooling (assumed to be the case) would not work.expressjs-framework/install-app > stage-packagespart.expressjs-framework/runtimeexists to cater to apt packages, since slices and apt packages cannot be used together by design. See [wishlist] separate stage-packages and stage-slices #785.libdirectory of apt/slice installs. Remove the offering until the issue is resolved. See different contents or permissions error #790.npm-include-nodeandnpm-node-versionfields.app/directory.apppath (e.g. .env files)