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

docs for components sharing state #571

Merged
merged 24 commits into from
Feb 19, 2022
Merged

Conversation

acivitillo
Copy link
Contributor

Adding documentation for components sharing state as discussed here: #561

I have added the 2 classic React examples: 1) synced input; 2) filterable list.

@rmorshea
Copy link
Collaborator

I'll take a look at this shortly. Thanks for the contribution!

@rmorshea
Copy link
Collaborator

Ultimately I think this content belongs under the "Managing State" section, but since it will probably be some time before I get around to writing more documentation, I'm going to accept this and move these docs around later. Thanks for writing this up!

@acivitillo
Copy link
Contributor Author

thanks for the quick review! I will work on it in the weekend

@acivitillo
Copy link
Contributor Author

OK I created the additional example for syncedinputs and moved the docs to managing-state

@rmorshea
Copy link
Collaborator

rmorshea commented Jan 16, 2022

It looks like there's just a minor style issue. If you've got your dev environment set up you should be able to just run isort . to fix the issue.

Also, let me know if you experienced any issues contributing to IDOM while working on Windows. I have a Windows machine I can use to test things out myself so it's very possible there are gaps in the contributor guide.

@acivitillo
Copy link
Contributor Author

ok I fixed the sorting of imports in the examples, it should work now

@acivitillo
Copy link
Contributor Author

I run the nox -s test_docs. Sphinx passes but then I get a fail on docker it seems, I don't understand this part.

image

@acivitillo
Copy link
Contributor Author

OK I had to fix 2 things to make this work and nox -s test_docs should now pass.

  1. I had to upgrade npm in Dockerfile and fast-json-path in docs/source/_cusom_js/package-lock.json and in src/client/package-lock.json

  2. I had to uncomment a 2nd Docker build in noxfile.py. nox is building the exact same docker image 2 times, from the looks of it the second build is not necessary and causes a bug (explained below)

Very interesting bug. The test_docs is building the same docker image 2 times, fix 1 above fixed the first build, but still the second build broke. The error is "Could not load /app/src/client/node_modules/fast-json-patch/index.mjs". This seems to happen because during the first docker build a folder node_modules is created inside the folder /client and for some reason this breaks the build. In fact, when I manually delete that folder it works.

noxfile.py Outdated
Comment on lines 251 to 260
# session.run(
# "docker",
# "build",
# ".",
# "--file",
# "docs/Dockerfile",
# "--tag",
# "idom-docs:latest",
# external=True,
# )
Copy link
Collaborator

@rmorshea rmorshea Feb 18, 2022

Choose a reason for hiding this comment

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

I'm not quite sure what's going wrong here. Were you seeing problems with this locally or in CI? This may be a windows specific problem since I haven't observed any issues in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got the same error in local and in CI: Could not load /app/src/client/node_modules/fast-json-patch/index.mjs.

But take a look at line 124, there is another docker build in the noxfile.py, so nox is building the same docker image 2 times.

I think the first time docker build creates node_modules folder in client and for some reason, this breaks the second docker build.

Copy link
Collaborator

@rmorshea rmorshea Feb 18, 2022

Choose a reason for hiding this comment

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

take a look at line 124

That's a separate command for running the docs locally in a docker container during development. It shouldn't be getting run during CI. This command here is testing whether or not the container can be built in CI which should run successfully. If not, then the container probably won't build when we try to deploy it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm gonna see what happens if I uncomment this. If it breaks in CI we can investigate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks to be working now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, then I will need to figure out how to run it locally w/o breaking. So the actual fix here was upgrading npm and fast-json-patch it seems.

noxfile.py Outdated Show resolved Hide resolved
@rmorshea rmorshea merged commit e8ae05a into reactive-python:main Feb 19, 2022
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