-
Notifications
You must be signed in to change notification settings - Fork 9
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(Template): Introduce template for react extensions #69
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats Luca, this is a huge step forward and I really look forward to using this!
There are a couple of minor fixes needed, and I'd like to discuss packaging and code organization:
packaging issue
the project is not configured to produce a correct package. when i run make dist
and then extract the tar.gz file (source distribution), i see
% tree dist/my_localstack_extension-0.1.0
dist/my_localstack_extension-0.1.0
├── my_localstack_extension
│ ├── extension.py
│ ├── frontend
│ │ ├── build
│ │ │ └── __init__.py
│ │ └── __init__.py
│ └── __init__.py
├── my_localstack_extension.egg-info
│ ├── dependency_links.txt
│ ├── entry_points.txt
│ ├── not-zip-safe
│ ├── PKG-INFO
│ ├── requires.txt
│ ├── SOURCES.txt
│ └── top_level.txt
├── PKG-INFO
├── README.md
├── setup.cfg
└── setup.py
Notice that it doesn't contain the built javascript sources.
code organization
Overall I wonder whether organizing the frontend code into the python module like this is the best option. currently we are "nesting" language projects into each other. there's the "outer" python project (indicated by the setup.cfg
and my_localstack_extension
module), and then the package.json
which is nested inside the module.
i haven't thought it through, but maybe it makes sense to have a frontend
or react
folder next to the my_localstack_extension
python module. then, during packaging, include the built files into the python distribution?
we would place the package.json
into the root folder next to the setup.cfg
, and during make dist
we'd first build the minified json with dependencies and include it into the dist.
I don't have the perfect answer here, it needs some exploring. Packaging requirements:
- any dependencies need to be served by the backend (i assume?) as static files
- needs to work with
EXTENSION_DEV_MODE=1
- should not include the source/project config files in the distribution after running
make dist
@@ -0,0 +1,43 @@ | |||
{ | |||
"name": "my_localstack_extension", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we should also parameterize from the cookiecutter parameters. perhaps {{ cookiecutter.project_name }}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, pardon 🙏
...lates/react/{{cookiecutter.project_slug}}/{{cookiecutter.module_name}}/frontend/package.json
Outdated
Show resolved
Hide resolved
...lates/react/{{cookiecutter.project_slug}}/{{cookiecutter.module_name}}/frontend/package.json
Outdated
Show resolved
Hide resolved
On one way I agree that having 2 languages intertwisted is a bit ugly; on the other way though your proposal doesn't convince me much either:
This aside I've addressed the other comment, thanks btw for the review @thrau 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work so far! I have a few suggestions for changes for the next review iteration.
I'd also suggest adding a default .gitignore
file that ignores (not exhaustive)
- build directories (dist, build, egg-info, ....)
- .venv
- frontend/.yarn
- frontend/node_modules
- ...
As discussed we're having some issues now due to the nature of how the extension dev mode works, which at the moment is incompatible with a src layout.
I was working on a workaround for this but we'll have to check if it's not better to potentially keep the flat layout instead
@@ -0,0 +1,29 @@ | |||
[metadata] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally think we should start using pyproject.toml
.
But maybe @thrau has more context on why we might still need the setup.cfg
& setup.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked with Thomas about this. The context here is that it's mostly legacy and he approves of moving toward unifying this via a pyproject.toml 👍
@Pive01 are you fine with me just pushing those changes directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
@@ -0,0 +1,20 @@ | |||
[metadata] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comments apply here of course as with the react template below
@@ -0,0 +1,32 @@ | |||
VENV_BIN = python3 -m venv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same arguments as below in the react template
templates/react/{{cookiecutter.project_slug}}/backend/{{cookiecutter.module_name}}/extension.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Dominik Schubert <dominik.schubert91@gmail.com>
@Pive01 I've pushed the changes necessary for a single One thing I'm not sure yet about is if we should really use |
Co-authored-by: Dominik Schubert <dominik.schubert91@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look really good! This is a super clear and explicit packaging. much easier to understand and work with. kudos both for this iteration!
my only remaining gripe is that i cannot use it out of the box with my node setup. my node installation comes with yarn 1.22.22, and when i run make install
, i get:
cd frontend && yarn install
error This project's package.json defines "packageManager": "yarn@3.2.3". However the current global version of Yarn is 1.22.21.
Presence of the "packageManager" field indicates that the project is meant to be used with Corepack, a tool included by default with all official Node.js distributions starting from 16.9 and 14.19.
Corepack must currently be enabled by running corepack enable in your terminal. For more information, check out https://yarnpkg.com/corepack.
make: *** [Makefile:30: install-frontend] Error 1
then, when i call yarn set version 3.2.3
in the repository, and run make install
again, i get:
cd frontend && yarn install
Usage Error: The nearest package directory (/tmp/foo/my-ui-extens/frontend) doesn't seem to be part of the project declared in /tmp/foo/my-ui-extens.
- If /tmp/foo/my-ui-extens isn't intended to be a project, remove any yarn.lock and/or package.json file there.
- If /tmp/foo/my-ui-extens is intended to be a project, it might be that you forgot to list frontend in its workspace configuration.
- Finally, if /tmp/foo/my-ui-extens is fine and you intend frontend to be treated as a completely separate project (not even a workspace), create an empty yarn.lock file in it.
$ yarn install [--json] [--immutable] [--immutable-cache] [--check-cache] [--inline-builds] [--mode #0]
make: *** [Makefile:30: install-frontend] Error 1
It would be great if we could either make the config compatible with these environments, or add instructions for default node installations.
Motivation
To allow a better integration between extensions and other LocalStack related products we can allow the creations of an extensions with e pre-built react app inside it that reuses the theme of our web app. This will allow a matching theme and easy access for developers to use react with it.
Content
index.html
while by default everything else will serve files from the build folder. You will still be able to add /hello handler and respond with what you want. Since for routing we usereact-router-dom
an url could be valid and invalid at the same time: once you land on the "/" page and the extension responds with theindex.html
then will be react itself to handle routing and not the extension, which means that navigating from the ui to pages such as /one and /dashboard will work because handled by react route, but going directly into them with a link will result in LocalStack try to resolve them and give you the file called "one" in your build folder (or if you have defined a handler for that route it will give you that).Other
Before merging this:
After merging this:
On the next major release we could also restructure both templates into a single folder to keep the project more clean but doing it now would break the
localstack extensions dev new
command as it would be a breaking change for older cli versions