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

Build hdf-converters with Rollup for html-mapper #6004

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

kemley76
Copy link
Contributor

As a way to simplify the usage of hdf-converter's hdf to html mapper, I have setup hdf-converters to be built using Rollup. This allows the template files (html, css, js) necessary for the html mapper to be bundled directly in with the hdf-converter's package. This removes the need for extra config for users of this mapper (no need to setup tailwind and move tailwind-elements files). Also, the main purpose of these changes is to allow for the mapper to work within the SAF CLI, which is now possible because it doesn't rely on Axios.

@kemley76 kemley76 mentioned this pull request Jul 17, 2024
@kemley76 kemley76 marked this pull request as draft July 17, 2024 15:39
Copy link
Contributor

mergify bot commented Jul 25, 2024

This pull request has a conflict. Could you fix it @kemley76?

Copy link
Contributor

mergify bot commented Jul 26, 2024

This pull request has a conflict. Could you fix it @kemley76?

@kemley76 kemley76 marked this pull request as ready for review July 26, 2024 16:32
"prebuild": "rimraf ../../dist/frontend",
"build:tailwind": "tailwindcss -i ./src/tailwind.css -o ./public/static/export/style.css --minify",
"build": "yarn build:tailwind && vue-cli-service build",
"build": "nx run build",
Copy link
Contributor

Choose a reason for hiding this comment

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

please swap out all lerna commands for nx. this will also require updating the release docs. i would also make sure everything works with the docker builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

please put all the project information into the package.json file instead of being a standalone config file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -8,7 +8,7 @@
],
"scripts": {
"backend": "yarn workspace heimdall-server",
"build": "lerna run build",
"build": "nx run-many --target=build --all",
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's possible to swap out the lerna config file for nx as well, that'd be great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally this still works if you rename it as .js instead of .cjs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -10,6 +10,7 @@
"outDir": "./lib",
"strict": true,
"esModuleInterop": true,
"resolveJsonModule": true
"resolveJsonModule": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary since already defined in the top level tsconfig.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

input: 'index.ts',
output: {
file: 'lib/bundle.js',
format: 'cjs'
Copy link
Contributor

Choose a reason for hiding this comment

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

something to keep in mind for us moving wholesale to esm is any explicit format specifications like this.

@@ -24,6 +23,9 @@ import {
IResultSeverity,
IResultStatus
} from './html-types';
import tailwindStyles from './style.css';
import htmlTemplate from './template.html';
import twElementsScript from './tw-elements.umd.min.js.txt';
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment explaining why this needs to be .txt here or wherever is most appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments have been added in several places.

Copy link
Contributor

mergify bot commented Jul 30, 2024

This pull request has a conflict. Could you fix it @kemley76?

Copy link

sonarcloud bot commented Aug 16, 2024

@kemley76
Copy link
Contributor Author

kemley76 commented Aug 16, 2024

State as of 8/16/24

These changes are all to get HTML export working in the SAF CLI: PR

Features added

I've made all the functional changes needed:

  • setting up rollup for hdf-converters
  • setting up the whole monorepo to handle rollup
  • ensuring HTML mapper uses bundled template files
  • setting up tailwind and tw-elements to generate bundled files
  • removing said file generation from frontend
  • swapping out lerna commands for nx commands to better manage the monorepo

What is left to do

  • ensure that docker containers can still build and function properly after changes to build system
  • ensure that the removal of postcss does not pose any issues. I think this might just require testing on various browsers?
  • replace lerna release system with nx release system and update documentation accordingly. Currently lerna is only in this project as part of the release process, but replacing it with nx is not trivial for getting nx to do automatic changelog generation for the first time using it.

Copy link
Contributor

mergify bot commented Aug 19, 2024

This pull request has a conflict. Could you fix it @kemley76?

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