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

Refactor for library / interface split #329

Merged
merged 49 commits into from
Jul 22, 2022
Merged

Refactor for library / interface split #329

merged 49 commits into from
Jul 22, 2022

Conversation

pkalita-lbl
Copy link
Collaborator

There are a lot of changes here so I'd be glad to walk anyone through them in more detail. Just reach out to me if that would be helpful for you.

The changes fall into these categories:

  • Adding typical modern JS development infrastructure (package.json, eslint, prettier, webpack and rollup configs). Removed manually copied-in third party dependencies (used to be in libraries folder); these are specified in package.json now.
  • JavaScript files that used to be in script/data-harmonizer have migrated into the three classes (DataHarmonizer, Toolbar, Footer) in lib. HTML snippets and CSS required by those classes are also in lib. lib/utils contains stateless functions that are used by one or more of the main classes. lib/images contain images used by the Getting Started carousel provided by Toolbar.
  • The "canonical" DataHarmonizer interface -- the one which will be distributed for offline use -- is now defined in web. This replaces main.html, main.js, and main.css that were previously at the top level of the repo. The templates directory has been moved to web/templates to emphasize that those templates are used by that canonical interface and are independent of the core library.
  • Schema files are now JSON and not JS (basically the var SCHEMA = part was removed).

The development workflow with this branch is:

  • Run yarn to install dependencies.
  • Run yarn dev to start the webpack dev server for the canonical DataHarmonizer interface on localhost:8080. This serves as interface for testing and debugging the core library components (in the lib directory) and that interface itself (the web directory).
  • Run yarn build:web to bundle the canonical interface. You can open web/dist/index.html in your browser to test the distributable bundle and verify it runs in "offline".
  • Run yarn build:lib to bundle the library components into lib/dist. That’s what will be published for downstream clients to use (there will be some follow-up work to document and automate that process). For now there isn’t much interesting to do with it, unless you really want to go the extra mile and consume your local DH project in another local project to see that working.

A few points that may warrant discussion:

  1. The fact that the images used in the Getting Started carousel need to be bundled into the core library is a bit awkward and could inflate downstream projects. It is tree-shakable, so if a client isn't using the Toolbar class, they won't incur the extra bundle size. Regardless, even for ease of updating I wonder if that carousel could be replaced by a link to documentation on the GitHub Wiki.
  2. You'll notice that web/templates/menu.js now includes information about the schema and export formats for each folder. This replaces the SCHEMA and EXPORT_FORMATS global variables that were previously used to manage that information. Instead of having the Toolbar class have knowledge about a specific file system structure, I added these two fields to the objects in menu.js that can either be inlined values (an object for schema or an array for exportFormats) or functions that provide such values. The dynamic imports in menu.js are a little awkward looking, but at least it keeps Toolbar's interface quite generic. Curious if anyone has alternate ideas.
  3. I have not updated scripts/linkml.py. A schema JSON file can be generated from the LinkML YAML on the command line with: gen-linkml --format json --mergeimports schema.yaml > schema.json.
  4. I attempted to leave the public interface of the DataHarmonizer class the same as it was before as much as possible. However I did attempt to remove any references to DOM elements in DataHarmonizer that it didn't create itself. That required replacing changeColVisibility with 4 more specific methods (showAllColumns, showRequiredColumns, showRecommendedColumns, showColumnsBySectionTitle) Personally I prefer this type of more explicit interface, but that's open to discussion. (It may also be worth in the future looking at what methods and properties on DataHarmonizer should be public and make the rest private.)

Fixes #312

pkalita-lbl and others added 30 commits June 15, 2022 11:07
const reassignment on line 920 `col = field.getColumn(this, col);` was causing my application to fail to load.
@ddooley
Copy link
Collaborator

ddooley commented Jul 1, 2022

I'll try to have a peek/runthrough tomorrow, and then follow up with questions.

@ddooley
Copy link
Collaborator

ddooley commented Jul 12, 2022

I checked it out and also consulted with the team and happy to see its 98% there! It seems like on my local test of web/dist/index.html, as well as the localhost http://localhost:8080/ yarn version:

  • validation can highlight erronious fields but the "next" button doesn't progress, and I can see some kind of javascript console error there.
  • I remember adding some css (with maybe an !important annotation or maybe it now needs that) just to get the column header required and recommended coloration to persist even when one scrolls down. At moment scrolling vertically seems to drop it. I suspect it may have to do with ordering of stylesheet introduction to final product.

I could find time this week to try to resolve those things. No other dev. planned for this week.

@pkalita-lbl
Copy link
Collaborator Author

Thanks for taking a look and testing! I'm happy to look into the issues you found but I'm not fully understanding the steps to reproduce the problems you saw. Here's what happens when I use the Next Error button:
https://user-images.githubusercontent.com/102547565/178542044-464e91c4-b834-4acf-9bf9-538dc5a89306.mov

And vertical scrolling:
https://user-images.githubusercontent.com/102547565/178542754-9c67e6c0-2bd7-4693-b980-633a872aadcd.mov

@ddooley
Copy link
Collaborator

ddooley commented Jul 12, 2022

Well that's working perfectly at your end! Are you on a Mac btw, and using Chrome? And was that via http://localhost:8080/ or via file explorer? e.g. file:///Users/damion/Documents/GitHub/DataHarmonizer/web/dist/index.html after the yarn web.build is done?

@pkalita-lbl
Copy link
Collaborator Author

Yes I am on a Mac and using Chrome. I recorded those while using the webpack-dev-server (localhost:8080) but I just tried the bundled version (opening web/dist/index.html) and saw the same behavior.

Maybe we should just do a quick call some time to see what's going on? It's entirely possible I didn't properly document some part of the workflow.

@ddooley ddooley mentioned this pull request Jul 12, 2022
@ddooley
Copy link
Collaborator

ddooley commented Jul 22, 2022

Hi Patrick, I see you've made this all up to date with DH v1.3.4 . I'm merging with master now!

@ddooley ddooley merged commit 865f615 into cidgoh:master Jul 22, 2022
@ddooley
Copy link
Collaborator

ddooley commented Jul 23, 2022

Should I just drop yarn.lock or try repairs in dependabot list:
https://github.com/cidgoh/DataHarmonizer/security/dependabot

@pkalita-lbl
Copy link
Collaborator Author

Ah sorry I don't have permission to view this repo's dependabot alerts. Don't remove yarn.lock that's important to keep checked in. If the issues have a fix dependabot will generally open a PR automatically, and I generally merge those without much scrutiny. Sometimes dependabot flags issues that don't have a fix published yet, and in that case I'm not sure if there's much else to do other than read what the issue is and see if this code could be affected by it.

@ddooley
Copy link
Collaborator

ddooley commented Jul 27, 2022 via email

kennethbruskiewicz pushed a commit that referenced this pull request Aug 21, 2023
Refactor for library / interface split
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.

Split project into library and interface components
3 participants