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

Split client and server into separate packages #223

Merged
merged 15 commits into from
Oct 8, 2020
Merged

Conversation

macfarlandian
Copy link
Collaborator

@macfarlandian macfarlandian commented Oct 7, 2020

Description of the change

Rearranges the code to create separate npm packages for the client and server applications, with a layer of multi-package tooling on top of it. It's a big PR, but the vast majority of it is just moving files to different directories with their contents unchanged. Salient changes:

  • existing dependencies and README sections have been distributed among the new packages as needed
  • root package.json and README now only address repo-wide concerns (linting, git hooks, scripts for running client and server together)
  • ESLint configurations are split up and nested
  • each new package has its own CI workflow definition
  • package versions have been bumped to match the latest release tag (and going forward we should keep them up to date when we release)

I have verified:

  • yarn dev works in each package on its own
  • the new multi-package dev and demo scripts in the project root work the same as they did before
  • testing and linting still work, including linting in the pre-commit hook
  • CI still works (there are even snazzy little badges in the READMEs now)
  • deployments still work (to staging)

Basically there should be no regression in application behavior or developer experience as a result of this change.

I think that I probably will not merge this PR until after the population over time feature is released — there aren't any PRs in flight, but because this lower priority I don't want it to cause any unforeseen issues. So I might merge in any additional changes that happen to support that release, but that shouldn't change anything significant here.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #217

Checklists

Development

These boxes should be checked by the submitter prior to merging:

  • Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@macfarlandian
Copy link
Collaborator Author

(note that it is still looking for the old CI tasks from master, that should stop happening once this is merged and we update the settings to stop requiring them)

Copy link
Member

@jessex jessex left a comment

Choose a reason for hiding this comment

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

I think this looks great, @macfarlandian! Sets a good standard for app development going forward. @jovergaag should take a look, too, before we merge, but don't hold up on me.


**Note: this is a one-way operation. Once you `eject`, you can’t go back!**
Do note that there should only be one `yarn.lock` file for the entire repository; if one is created in your new package directory, you should delete it, make sure the directory is properly listed in the `workspaces` list, and re-run `yarn install`.
Copy link
Member

Choose a reason for hiding this comment

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

So although there are separate package.json files per package, there is only a single resulting yarn.lock file. And the possibility of conflicts between the two files gets smoothed out by Yarn Workspaces?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes ... if there are redundancies it "hoists" them to the root to avoid duplicate installs, and if there are multiple versions that's fine because that happens all the time in node anyway. We will get more mileage out of the hoisting once we have multiple React applications in here

Copy link

@jovergaag jovergaag left a comment

Choose a reason for hiding this comment

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

This is awesome. Very clean, definitely setting the standard! We will try to start moving the Pulse dashboard in this direction over time.

@@ -1,5 +1,7 @@
{
"extends": ["react-app", "airbnb", "plugin:prettier/recommended"],
// unlike .eslintignore, these values will cascade

Choose a reason for hiding this comment

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

Cool.

@jovergaag
Copy link

@macfarlandian PS your code is beautiful 😻

@macfarlandian macfarlandian merged commit 0b17ce8 into master Oct 8, 2020
@macfarlandian macfarlandian deleted the package-reorg branch October 8, 2020 18:56
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.

Reorganize public-dashboard repo for multiple packages
3 participants