refactor: Remove the primer-app workspace, flatten to one project.
#547
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We do this for the following reasons: * We want to get rid of the `primer-types` package, which duplicates types exported from the Primer API. However, in the process of doing this, we ran into some long-standing upstream issues (described in #545) which seem to be related to using React Hooks defined in one package but used in another. * I originally chose this particular project organization so that we could decouple Storybook.js from the frontend application, because I was worried that, if they were integrated into the same package, issues with Storybook and new versions of React would hold up development of the frontend. However, we've already successfully made the transition from React v17 to v18, and Storybook.js was ready before we were, so this hasn't been an issue in practice. * Interactive development would be simpler, and possibly faster, if we unified the packages. The refactoring here could probably be improved, but this was the simplest first step I could come up with. The next most obvious step is to remove the `primer-app` workspace and move to just a flat package. Note that I have *not* actually removed the `primer-types` types in this commit: I've only moved them into a single `Types.ts` file.
dhess
changed the title
primer-app workspace, flatten to one project.
Deploying with
|
| Latest commit: |
bb37048
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fe86334b.primer-app.pages.dev |
| Branch Preview URL: | https://dhess-flatten.primer-app.pages.dev |
|
edit: Fixed in bb37048. |
georgefst
approved these changes
Oct 10, 2022
This fixes the issue described here: #547 (comment) It does so by configuring `direnv` to write its cache for this project in `~/.cache/direnv/layouts`. This is a bit intrusive, but without this workaround, `vite` commands search the local `.direnv` cache for dependencies, and it adds about 10 seconds or more to every `vite` command. Note that the fix comes from here: https://github.com/direnv/direnv/wiki/Customizing-cache-location Before falling back to this workaround, I also tried the following Vite configuration tricks, but none of them worked: https://vitejs.dev/config/server-options.html#server-watch https://vitejs.dev/config/dep-optimization-options.html#optimizedeps-exclude There are multiple (somewhat) related issues currently filed on Vite which suggest that convincing Vite not to look everywhere in the project root directory is a known issue: https://github.com/vitejs/vite/pull/8778/files vitejs/vite#7363 vitejs/vite#8341
|
I'm going to merge this even with the known local Storybook development issue. I will file an issue and come back to it when I have time. Please let me know if this is a priority for your particular development process. edit: Tracked in #548 |
This was referenced Oct 10, 2022
dhess
added a commit
that referenced
this pull request
Oct 18, 2022
What we initially thought was a Storybook issue (see #548) turns out to be some sort of Vite HMR problem with the way we were exporting our component classes. See #546 (comment) for details of how the issue manifested in Storybook, and #548 (comment) for details of how one could trigger it from `pnpm watch` on the application itself. Somehow this issue only manifested once we merged our components into a single project alongside the application code in #547, possibly because we're now importing the components directly from the filesystem, rather than via an intra-workspace package dependency. It's not precisely clear what is happening here, nor why the changes in this commit fix it, but given the constant parade of module- and import-related issues endemic to the JavaScript ecosystem, frankly I'm not sure we really care to dive in and figure it out. Suffice to say that, in addition to fixing the issue at hand, this commit also simplifies the definition of our components by removing a boilerplate-y `index.tsx` file that simply re-exports the definitions defined in the "home" `.tsx` file. Closes #548
dhess
added a commit
that referenced
this pull request
Oct 18, 2022
What we initially thought was a Storybook issue (see #548) turns out to be some sort of Vite HMR problem with the way we were exporting our component classes. See #546 (comment) for details of how the issue manifested in Storybook, and #548 (comment) for details of how one could trigger it from `pnpm watch` on the application itself. Somehow this issue only manifested once we merged our components into a single project alongside the application code in #547, possibly because we're now importing the components directly from the filesystem, rather than via an intra-workspace package dependency. It's not precisely clear what is happening here, nor why the changes in this commit fix it, but given the constant parade of module- and import-related issues endemic to the JavaScript ecosystem, frankly I'm not sure we really care to dive in and figure it out. Suffice to say that, in addition to fixing the issue at hand, this commit also simplifies the definition of our components by removing a boilerplate-y `index.tsx` file that simply re-exports the definitions defined in the "home" `.tsx` file. Closes #548
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This takes #546 one step farther and removes workspaces altogether.