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

Move to Craco from react-app-rewired #3546

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

bryanculver
Copy link
Member

@bryanculver bryanculver commented Apr 6, 2023

Closes: #DNE

What's Changed

The problem this was trying to solve is Jest (the built-in testing library from Create React App) is not aware of the Webpack aliases we establish in jsconfig.json (see: ci failure). For Jest to become aware of these, it needs a slightly different specification with moduleNameMapper. So as to not generate this twice, I looked into a library called react-app-alias which is a listed plugin for react-app-rewired (our current library react-app-rewire-alias is the former package to react-app-alias). But, its integration with Rewired seemed a little less elegant mention Craco, which also happens to be a listed alternative to react-app-rewired (see: link). Rewired also is mentioned to be lightly maintained (last release Feb 2022). Craco was updated last month and has seen several updated last year. All other listed "more supported" libraries on Rewired's README are now archived, all signs pointing to Craco.

As you'll see with the changes below and the diff it's actually a more elegant way to customize the config for our needs.

Changes:

  • Removed react-app-rewired in favor of @craco/craco
    • Newer, simpler, more maintained
  • Switched from react-app-rewire-alias in favor of react-app-alias
    • The newer incantation from the same maintainer
  • Introduced (and semi-vendored) react-app-alias-ex
  • Moved the alias path building to jsconfig.paths.json
    • This is the documented convention from react-app-alias
    • This also feels nicer that should we need to extend the compiler config further it doesn't need to be in our base and that the only thing Django is responsible for is generating a list of aliases and paths
  • Copy jsconfig-base.json to jsconfig.paths.json for CI
  • Fixup import paths which are starting from a base path, instead use relative
  • Semi-vendored react-app-alias-ex
    • I have opened a bug with the maintainer for more details but with this cut-over I still ran into an issue running Jest but this time because it couldn't find node_modules.
    • Instead of vendoring the entire package, I copied the existing aliasJest function and patched what I felt neccessary (use the Jest default and prefer relative paths that Jest leaned twoards in their documentation)
    • If and when this gets fixed it should be pretty simple to remove.

CI tests are passing now πŸŽ‰! But I had to comment a bunch out because we (me) have been merging code in to get some of the bedrock updated for the site but I think now's the time we can start getting tests going for UI 2.0!

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

@bryanculver bryanculver requested a review from a team April 6, 2023 03:24
@bryanculver
Copy link
Member Author

Passing: https://github.com/nautobot/nautobot/actions/runs/4625253586/jobs/8180844239

Now just to get next-ui-2.0 caught up to next.

Copy link
Contributor

@HanlinMiao HanlinMiao left a comment

Choose a reason for hiding this comment

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

Nice, it looks good based on my limited understanding. But the UI test is passing, so a great step forward.

@@ -2,7 +2,7 @@ import { render } from "@testing-library/react";
import AppFullWidthComponentsWithProps from "../AppFullWidthComponents";

// Set NautobotApps to {}; since we are not testing for plugin integration with nautobot
jest.mock("src/app_imports", () => ({
jest.mock("../../../app_imports", () => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the meaning of replacing src with ../../..? Same comment for similar changes in other files.

Edited: Wait does jest prefer relative path? I saw that in the PR description just now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a limitation in the alias/packing library we are using: oklas/react-app-alias#73

Since it doesn't know where "base" is, you can't define paths relative to it. Before we had said the top-level directory (nautobot/ui) was "base" with '.'. So src/app_imports was resolved relative to it, nautobot/ui/src/app_import. Because of that bug we can't do that. However we should either be using our registered aliases @components, @views, etc or be knowingly using relative paths. Maybe we should add @base: '.' and @nautobot_src: 'src'? But at the moment we won't be accessing files in the top root very often.

// expect(alert.innerHTML).toBe("Current route is /test");
// const childrenDom = screen.getByTestId("child-div");
// expect(childrenDom.innerHTML).toBe("Child Text");
expect(true).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Expect true to be trueπŸš€

Base automatically changed from u/bryanculver-nodejs-linting-formatting to next-ui-2.0 April 6, 2023 18:16
Fix Jest testing

Remove should be ignored paths file as this is autogenerated

Scope creep avoid get tests passing so we can demonstrate it's working.

Prettier...

Add base paths for testing

No cached example, just copy the template in CI

Mock would prefer a file exists for app_imports.js
@bryanculver bryanculver force-pushed the u/bryanculver-swap-rewired-for-craco branch from 5769c19 to bed0ebd Compare April 6, 2023 19:25
@bryanculver bryanculver merged commit d23bfdb into next-ui-2.0 Apr 7, 2023
13 of 17 checks passed
@bryanculver bryanculver deleted the u/bryanculver-swap-rewired-for-craco branch April 7, 2023 18:26
@bryanculver bryanculver self-assigned this Apr 11, 2023
@bryanculver bryanculver added the type: housekeeping Changes to the application which do not directly impact the end user label Apr 11, 2023
bryanculver added a commit that referenced this pull request Apr 25, 2023
* create list templete

* create add form view templete

* refactor and create NautobotInput Component

* move sites/ to dcim/sites/

* move react back one folder

* move react back one folder

* new changes

* new changes

* get model fields

* copy react files from plugin into nautobot frontend

* fix menu

* Work in progress on table

* json api

* Add get table fields

* fix table header

* Use absolte path

* slugify python

* Field groups

* Account for status and tags

* add react to example plugin

* push all code

* nautobot.2319 React UI components update

* add docker-compose.nodejs.yml to run an nginx reverse proxy for nautobot and nodejs

* remove unused ui prototype

* update to work with nextjs

* Revert "remove unused ui prototype"

This reverts commit 7bccab7.

* make react work?

* add object detail view

* remove comment

* remove comments

* add links to object detail view

* fix region

* Implement basic module federation for webpack

- Publishes `nautobot` module as `remoteEntry.js`
- Implemented webpack `config-overrides.js` using `react-app-rewired` as a dev dependency

* remove nextjs ui

* remove plugin file copy

* optimize dev docker environment

* Update react app build output paths

Output js to static/reactjs
Output css to static/reactcss

* use global router

* add postbuild script to move files to nautobot

* add navmenu headers

* convert prototype to nextjs

* fix some imports

* update to use cross site api calls

* remove nginx config

* update nautobot dev server to use uwsgi instead of runserver

* consolidate frontend pages, update django routing for react

* add plugin full width html fragment support

* fix detail page layout

* add plugin routes

* expose django's login/logout routes

* YEET

* Fixing some lints and bugs.

- Removed bad import in router
- Fixed linting errors for unused imports
- Fixed URL route on table item
- Improved Docker compose file to mount node_modules as a separate volume

* fix permissions issue on docker-compose in linux

* fix linux docker nodejs permissions (really)

* Make detail view generic

* stashing

* add paginator for list view

* yolo

* fix nautobot brand image, add some better looking icons

* stashing

* stashing

* replace react views with base.html that's derived from the react app's index.html

* Something broke but this is progress

* CustomViews and Plugin Components

* fix bug in detail view when json key name starts with underscore

* basic management command to generate ui

* tweaks

* Tweaks and update

* fixes and starts

* update dockerfile to match next

* update poetry lockfile

* linting and minor bug fixes

* clean up imports

* fix bug

* added nginx container in nodejs.yml and made nodejs serve up /static/js/bundle.js (#3057)

Co-authored-by: Hanlin Miao <hanlinmiao@Hanlins-MBP-2.home>

* fix runserver database exhaustion with nginx

* increase keepalive timeout on nginx connection

* Revert "increase keepalive timeout on nginx connection"

This reverts commit d3aabdd.

* increase keepalive timeout on nginx connection

* clean up api for plugin react components

* add web_url to all valid serializers

* remove debug logging

* add login view

* fix js error

* Implemented generic create view for React app.

- Requires the nested serialization changes.
- Implemented nested serialization using `?depth=`
    - Revert nested serializer and `brief=True` pattern
    - Replace it with `depth=?` arguments to views and rendering forms from JSON schema using `drf-react-template-framework`
    - Commented out all `brief=` and associated code
- Updated `RegionSerializer` to have `parent` field fallback to default.

* First pass at #3204 (#3252)

* Some low-hanging fruit fixes to get us closer back to next

* next-ui-2.0 cleanup and commenting (#3285)

* Remove unused code added to example_plugin

* Add a bunch of TODO comments

* More TODO comments

* Black

* Flake8

* Markdownlint

* Pylint

* Removing some more stuff that's not needed at present

* Revert a few things back to baseline

* One more deletion

* Fixes to example_plugin and example_plugin_with_view_override

* Update comments and address a couple of TODOs

* More TODO updates

* web_url fixup

* Added a `nautobot-server build_ui` command (#3306)

- This command does the following from the `nautobot/ui` directory
  - Generates the `app_imports.js` file from any installed plugins
  - Runs `npm install` when `--npm-install` is passed
  - Runs `npm run build`
- This command is now also called by `nautobot-server post_upgrade`, which calls it with `--npm-install`
- Updated the `config-overrides.js` to not hash JS or CSS files so that their raw filename can be stored in static files and referenced using `{% static %}` (e.g. `{% static 'js/main.js %}`) without having to overload the `base.html` template for the web UI
- Updated `nautobot/core/templates/base.html` to link directly to the static files for JS and CSS that get pulled in from the UI build environment by `nautobot-server collectstatic` 
- Parameterized the API key needed for now to read from `NAUTOBOT_API_TOKEN` environment variable
- `app_imports.js` now renders with relative paths vs. absolute paths making commits that happen compatible across Docker-local and production use-cases.
- Restored the `/plugins/` route and the `InstalledPlugins.jsx` module
- The `build` command runs `npm` silently by default to hide the debug/warn/error messages unless `-v 2` or `-v 3` are passed.
- There is some `console.log` output that is now not displayed unless `NAUTOBOT_DEBUG` environment variable is set. The `build_ui` command will also set this if verbosity is desired.
- Implemented a new `invoke version` task to align versions of core with UI apps.
  - Will bump both `pyproject.toml` and `package.json` to keep them in sync.
  - If no argument is provided, displays the versions instead.
- Also added `NAUTOBOT_UI_DIR` into `nautobot.core.settings` so that it can be easily reused
- Moved `nautobot_ui` to `nautobot/ui`
- `nautobot/ui` is now included in the package build
- Added a basic README file back to `nautobot/ui`
- Renamed all UI references of "plugin" to "app"

After using `build_ui` it's possible to serve the entire application using Django now without requiring `node`. Of course for development `node` will still be desirable due to live rendering.

* setState for islogged in

* Revert "setState for islogged in"

This reverts commit 596d9c1.

* remove ourrunserver reference in docker compose (#3366)

* UI 2.0 remove nginx container (#3367)

* Initial removal of react-bootstrap and addition of nautobot-ui

* Clean up devDependencies

* Simplify tasks.py

* Updates from feedback

* Add node_modules ignore

* move 'api/' ui related urls under 'api/ui/' (#3385)

* move 'api/' ui related urls under 'api/ui/'

* flake8

* address PR feedback

* exclude get-menu api endpoint from OpenAPI schema and add documentation for UI-related API endpoints

* flake8

* mdlint

* elaborate UI-related endpoints documentation

* update mkdocs.yml

---------

Co-authored-by: Hanlin Miao <hanlinmiao@Hanlins-MBP-2.home>

* Logged in state for nautobot-ui (#3356)

* Update from fix nautobot/nautobot-ui#9

* Black

* Hadolint

* More hadolint

* UI docker dev environment tweaks (#3408)

* Don't copy node_modules and UI build when rebuilding docker image

* Capture npm output and print it in case of failures

* Create docker volume for node_modules, change container startup, update docs

* nautobot-ui is public now; can use node:18-slim

* Add TODO comments

* Add unittest for all react components (#3404)

* Add test for component common

* common test case

* Done testing core

* Add test for BSTableItem Component

* Add partial views test

* Add test for views

* add doc

* run ui unittest

* Apply suggestions from code review

Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>

* fix markdown

* update ReadMe

* Update nautobot/ui/README.md

Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com>

* Merge conflicts

* Update README.md

---------

Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>
Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com>
Co-authored-by: Bryan Culver <bryan.culver@networktocode.com>

* UI 2.0 Cleanup (#3444)

* Provide a way to refresh CSRF without shortcutting CSRF protections

* Session endpoint

* Stashing to switch between computers

* more stashing

* yeeet

* This is going to be a lot

* Updates

* Updates from feedback

* Black & flake8

* Fix indention

* Remove top-level package lock

* Revert bad merge

* UI 2.0: Cleanup Router Links, Import Font as Source (#3482)

* Cleanup Router Links

* Move font to built-in instead of served via Google Fonts.

* Remove unneccessary import

* UI 2.0: Introduce Prettier, Use ESLint for Formatting and Linting (#3531)

* Move to craco (#3546)

Fix Jest testing

Remove should be ignored paths file as this is autogenerated

Scope creep avoid get tests passing so we can demonstrate it's working.

Prettier...

Add base paths for testing

No cached example, just copy the template in CI

Mock would prefer a file exists for app_imports.js

* Add New UI Toggle (#3615)

* Add New UI Toggle

* Add changelog fragment.

* Prettier

* Fix corrupted package-lock.json (#3618)

* Fix corrupted package-lock.json

* Add changelog fragment.

* Remove duplicate package for nautbot-ui

* Get tests passing for UI 2.0 (#3632)

* Get tests passing for UI 2.0

* Add changelog fragment.

* Updates from feedback

* Exception handling

* Fix CI step merge

* Pylint

* Fix changes from next

* UI 2.0 Implement GenericView component (#3626)

* [UI 2.0] Implement GenericView component

* [UI 2.0] Add missing objectData prop to GenericView in ObjectRetrieve view

* [UI 2.0] Remove Current route is ... debug message

* Move popover to it's own component.

---------

Co-authored-by: Bryan Culver <bryan.culver@networktocode.com>
Co-authored-by: Bryan Culver <me@bryanculver.com>

* πŸ‘Ÿ

---------

Co-authored-by: Timizuo <timizuoebideri@gmail.com>
Co-authored-by: Chris Thant <chris.thant@networktocode.com>
Co-authored-by: Gary Snider <gary.snider@networktocode.com>
Co-authored-by: jathanism <jathan@gmail.com>
Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>
Co-authored-by: Hanlin Miao <hanlinmiao@Hanlins-MBP-2.home>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>
Co-authored-by: Timizuo <94907097+timizuoebideri1@users.noreply.github.com>
Co-authored-by: Norbert Mieczkowski <117445994+norbert-mieczkowski-codilime@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: housekeeping Changes to the application which do not directly impact the end user
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants