Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move to vite #342

Merged
merged 25 commits into from
Apr 28, 2023
Merged

Move to vite #342

merged 25 commits into from
Apr 28, 2023

Conversation

twinkarma
Copy link
Collaborator

@twinkarma twinkarma commented Apr 13, 2023

Resolves #309

Changes to code and build chain

  • The @ alias can still be used when doing module imports but file extensions should now be used when importing .vue files e.g.
    • Before: import DeleteModal from "@/components/DeleteModal"
    • Now: import DeleteModal from "@/components/DeleteModal.vue"
  • For code that is intended to run on the browser, e.g. in all .vue files, imports should use the ECMA 6 compliant import command and not node/commonjs's require
    • Apart from for code that is run directly by node, e.g. scripts used in the build chain, config files and test files used by build tools that run on node (e.g. vuepress or cypress). ECMA script is still in experimental support as of current node version v18.8.0
  • Updated chart.js to v4 and vue-chartjs to v5, the formatting of options changed and so are updated accordingly
  • Removed vue-chart-3 as it looks like it's not being used
  • Vite builds all assets to frontend/dist/static/assets directory
  • Template for the main page now located at frontend/base_index.html , this gets merged with frontend/dist/manifest.json and outputs to frontend/templates/index.html so that the correct hashed js and css files are imported
  • Main page template relies on Django's setting.DEBUG value for switching between using Vite dev server or using vite's built assets (for deployment)
  • Added explanation on the frontend to the documentation

Changes to frontend testing

  • Updated to the latest version of cypress
  • Since ECMA6 was not fully supported by Jest, I'd taken the opportunity to move our frontend test code over to cypress and standardise all of our UI testing. npm run test:component
    • Component tests are placed in /frontend/tests/component and should have the extension .cy.js
  • All non-ui tests now use vitest which is almost a drop-in replacement to Jest npm run test:unit
    • Unit tests are placed in /frontend/tests/unit and retains the previous extension .spec.js
  • All frontend and integration test fixture files should be placed in /examples folder
  • Integration tests specs are now located in /cypress/e2e, this is the standard convention for updating to their latest version

@davidwilby
Copy link
Contributor

Also for annotation statistics, what's the Y axis??

There isn't one, it's a jittered 1D scatter plot.

- preserves the {{settings.APP_NAME}} placeholders which need to be resolved at runtime by Django
- uses the right favicon in production mode
…an be overridden by setting DJANGO_DEBUG=1 in the environment

Previously we were hard coding DEBUG=True in all cases, which is probably a security issue
…om Vite build vs the old Webpack.

Also, the switch to Vite un-breaks the npm build on ARM64, so we can now use $BUILDPLATFORM for the nodebuilder stage rather than having to hard code amd64
@ianroberts
Copy link
Member

I've done my best to fix up the Docker image build to work with the new Vite artifacts. There were a couple of wrinkles in this, most notably that it seems up until now we've always been running with DEBUG = True hard coded in the base settings and never overridden in any of the other settings files. The index.html template now assumes the Vite dev server whenever settings.DEBUG is true, which broke the docker deployment as even the production page was linking http://localhost:5173 for the assets. I've changed the deployment.py settings file to default to DEBUG = False but I'm unsure what the setting needs to be in any of the other environments.

Maybe we need a separate setting for vite dev server vs static assets, since we want to be able to run the various test suites against the production assets but is there anything else in the tests that assumes DEBUG = True?

@davidwilby
Copy link
Contributor

Changes to be made to test settings:

  • remove one of settings/test.py or settings/docker-test.py since they're functionally identical. We don't need to set TELEMETRY_URL, TELEMETRY_PATH or TELEMETRY_ON since these are set to usable values in settings/base.py

@ianroberts
Copy link
Member

Maybe we need a separate setting for vite dev server vs static assets, since we want to be able to run the various test suites against the production assets but is there anything else in the tests that assumes DEBUG = True?

This sounds like the best way to go.

@ianroberts
Copy link
Member

Also anyone have any idea why the workflows aren't running for this PR?

@davidwilby
Copy link
Contributor

Also anyone have any idea why the workflows aren't running for this PR?

There's a merge conflict.

@github-actions
Copy link

github-actions bot commented Apr 27, 2023

Jest Coverage

File % Stmts % Branch % Funcs % Lines Uncovered Line #s
All files 86.66 85.18 53.33 86.66
File % Stmts % Branch % Funcs % Lines Uncovered Line #s
All files 86.66 85.18 53.33 86.66
_jrpc 94.04 90.9 83.33 94.04
_ index.js 94.04 90.9 83.33 94.04 29-30,38-40
_utils 83.98 83.72 33.33 83.98
_ annotations.js 97.72 73.91 100 97.72 35-36
_ dict.js 88.88 83.33 100 88.88 3-4
_ index.js 73.6 100 14.28 73.6 9-17,28-29,43-53,64-65,76-82,93-94

Copy link
Member

@ianroberts ianroberts left a comment

Choose a reason for hiding this comment

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

Looks good to me as far as I can tell, aside from the discussion we had yesterday about moving the "dev server vs static assets" toggle to be independent of the the Django-wide DEBUG flag.

@twinkarma
Copy link
Collaborator Author

Looks good to me as far as I can tell, aside from the discussion we had yesterday about moving the "dev server vs static assets" toggle to be independent of the the Django-wide DEBUG flag.

I've added a FRONTEND_DEV_SERVER_USE variable that is set to True in base, test and integration and False in the other settings files.

@twinkarma twinkarma requested a review from ianroberts April 28, 2023 14:13
Copy link
Member

@ianroberts ianroberts left a comment

Choose a reason for hiding this comment

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

LGTM. Should cypress be in devDependencies rather than dependencies for the frontend or does it not make a difference?

@twinkarma
Copy link
Collaborator Author

LGTM. Should cypress be in devDependencies rather than dependencies for the frontend or does it not make a difference?

Practically it should not make a difference as during deployment we only use the static files generated from vite and that will only have libraries which are directly imported from main.js. But it's better practice to properly separate them so I'll change this.

@twinkarma twinkarma merged commit 9edf802 into dev Apr 28, 2023
@twinkarma twinkarma deleted the move-to-vite branch April 28, 2023 15:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants