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

Base setup for using react-uswds in the veda-ui #1029

Merged
merged 19 commits into from
Jul 12, 2024

Conversation

dzole0311
Copy link
Collaborator

@dzole0311 dzole0311 commented Jul 2, 2024

Related Ticket: #1043

Description of Changes

This is a base setup to start using react-uswds inside of the veda-ui

Example usage

To use the Card component from react-uswds, we can simply import it and use it as is from react-uswds, with the default styles applied.

Notes & Questions About Changes

{Add additonal notes and outstanding questions here related to changes in this pull request}

Validation / Testing

{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}

@dzole0311 dzole0311 requested a review from hanbyul-here July 2, 2024 13:33
Copy link

netlify bot commented Jul 2, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit a6b1ff9
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6690224868520e00088e029f
😎 Deploy Preview https://deploy-preview-1029--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dzole0311 dzole0311 requested a review from sandrahoang686 July 2, 2024 13:56
postcss.config.js Outdated Show resolved Hide resolved
@hanbyul-here
Copy link
Collaborator

One thing that blocks me from merging this PR is how slow the dev server is. First, are you also experiencing the slow dev server problem @dzole0311 ? Would it be possibly related to warning from the transformer that @sandrahoang686 pointed out? @parcel/transformer-postcss: WARNING: Using a JavaScript PostCSS config file means losing out on caching features of Parcel - but I am unclear how I can migrate postcss.config.js to .postcssrc with plugins configuration now. (parcel-bundler/parcel#5699) I will continue this tomorrow

@dzole0311
Copy link
Collaborator Author

dzole0311 commented Jul 9, 2024

One thing that blocks me from merging this PR is how slow the dev server is. First, are you also experiencing the slow dev server problem @dzole0311 ?

I pushed a change that was on my local branch for 979, thus I did not notice the dev server being slow over here. The issue was this line, which imported the whole design uswds system. Benchmarking without it on 288 gives me 18s for a yarn clean + yarn serve, and each subsequent serve is 2s due to caching. As comparison, our main branch builds in half that time after a yarn clean. Given that we're switching to a new design library, I think this is acceptable? We could find ways to speed this up more as we go.

I think the point above also means that we don't need to use purgeCSS anymore since we would import packages per component. Sorry for using your draft PR for this @hanbyul-here , but I rebased the new changes there and removed the purgeCSS and it results in a way smaller CSS.

@parcel/transformer-postcss: WARNING: Using a JavaScript PostCSS config file means losing out on caching features of Parcel

Could we try to get rid of this warning via the new "Optimize USWDS css" issue, together with the uswds notifications while compiling? I tried changing the latter here, but it seems to have no effect.

@hanbyul-here
Copy link
Collaborator

Ah, thank you so much for redirecting the configuration @dzole0311 ! (Now I also can see that the verbose warning comes from the instruction with how to turn it off 😓 ) - I misunderstood how uswds design system (or even just scss) works, in a way that we need a whole scss to make use of its full capacities.

One last thing I am checking is if any change we are introducing is interrupting the library build ( I should just make it as a part of the build command). I can't seem to make the build work with the new version of Parcel. Sorry to keep dragging the pr, but this seems to be a big enough issue to be a blocker. I am looking at it, but lmk if you have a clue about it. This is the error message I get when I run publishlib with [email protected] . I am not sure how accurate this error is, since we didn't have any problem with aliases with the previous version of Parcel for library build.


@parcel/core: Failed to resolve '$components/common/layout-root/useSlidingStickyHeaderProps' from './app/scripts/components/common/catalog/index.tsx'

  /Users/hanbyul/ds/git/veda-config-ghg/.veda/ui/app/scripts/components/common/catalog/index.tsx:9:8
     8 |   useSlidingStickyHeaderProps
  >  9 | } from '$components/common/layout-root/useSlidingStickyHeaderProps';
  >    |        ^
    10 | 
    11 | import {

@parcel/resolver-default: External dependency "$components" is not declared in package.json.

  /Users/hanbyul/ds/git/veda-config-ghg/.veda/ui/package.json:111:3
    110 |   },
  > 111 |   "dependencies": {
  >     |   ^^^^^^^^^^^^^^
    112 |     "@codemirror/lang-markdown": "^6.1.1",
    113 |     "@codemirror/state": "^6.2.1",

  💡 Add "$components" as a dependency.

@dzole0311
Copy link
Collaborator Author

dzole0311 commented Jul 9, 2024

I can't seem to make the build work with the new version of Parcel

Hmm, yeah I ran the buildlib now and I do get that error. I also noticed errors (albeit not the same) with the previous Parcel v2.7 and using the main branch. M̶i̶g̶h̶t̶ ̶b̶e̶ ̶r̶e̶l̶a̶t̶e̶d̶ ̶t̶o̶ ̶s̶o̶m̶e̶t̶h̶i̶n̶g̶ ̶o̶n̶ ̶̶m̶a̶i̶n̶̶?̶ ̶I̶ ̶r̶e̶b̶a̶s̶e̶d̶ ̶t̶h̶i̶s̶ ̶b̶r̶a̶n̶c̶h̶ ̶w̶i̶t̶h̶ ̶̶m̶a̶i̶n̶̶ ̶a̶ ̶c̶o̶u̶p̶l̶e̶ ̶o̶f̶ ̶d̶a̶y̶s̶ ̶a̶g̶o̶.̶

EDIT: There are only TS errors on main, but the library gets built fine.

@hanbyul-here
Copy link
Collaborator

I looked into the problem more today. I gradually increased the minor version of Parcel and its related dependencies to see when the bug was introduced. The problem appeared from Parcel 2.9.0, when Parcel rewrote its resolvers extensively.https://parceljs.org/blog/v2-9-0/ I dipped around many things, but eventually, Parcel doesn't compile aliases when the target is not a web app. I found a similar issue from the repo: parcel-bundler/parcel#9519

I am kind of blocked here now 🤔 Hopefully I will have more ideas tomorrow.

@dzole0311
Copy link
Collaborator Author

dzole0311 commented Jul 10, 2024

Thanks for looking into it @hanbyul-here , I'm throwing some ideas below.

One option we could try is to downgrade to Parcel v2.7 and try the errorRecovery option for "@parcel/transformer-css" (parcel-bundler/website#1071) and see if Parcel parses the CSS without problems. If this works then we could ditch the postcss transformer and only keep the default Parcel transformer. As a reference this is what the default CSS transformer was throwing for the USWDS css: https://app.netlify.com/sites/veda-ui/deploys/668401aec64e5500083cc372#L336

A second alternative would be to downgrade to Parcel v2.7 and stop using postcss to fix the invalid CSS from USWDS, and to write a custom Parcel Transformer that patches the css using Parcel v2.7. Something like this: parcel-bundler/lightningcss#39 (comment). The downside is that I can't tell how many invalid selectors we have to fix.

@hanbyul-here
Copy link
Collaborator

Thank you @dzole0311 for the suggestion! I think all your suggestions make a lot of sense. I was wondering if there is any way not to lock ourselves to Parcel 2.7, since 2.7 is quite old anyway at this point. Your comment made me think about using Parcel's Resolver API to resolve aliases. I made a PR: #1045

**Related Ticket:**  (Not a ticket) #1029 

### Context
To use the USWDS Design System, we had to update Parcel to 2.12.
(Because of invalid CSS selectors that Parcel will complain about.
@dzole0311 described the problem very well here:
#1029 (comment))

While trying to build the library with Parcel 2.12, I faced the problem
of Parcel failing to build the library with an error like `add
'$components' as a dependency`. This error made me think that something
related to aliases is not working for library build with the new version
of Parcel.

We are bumping quite a number for Parcel (from 2.7 to 2.12), so I
gradually bumped up the minor version of Parcel and its related packages
to see where this error was introduced. The problem appeared in Parcel
2.9.0 when Parcel rewrote its resolvers
extensively.https://parceljs.org/blog/v2-9-0/ There seem to be other
people who are facing alias-related problems with targeting Node:
parcel-bundler/parcel#9519

**In short, Parcel 2.12 seems to have a problem building a library with
aliases. (Or at least our set up doesn't seem to work for library
build)**

### What this PR does

Parcel offers the api for custom Resolver :
https://parceljs.org/plugin-system/resolver/
From what I understand, I can make my own translation of dependency such
as `import x from ...` through Resolver. Using this Resolver api, I
added an alias-resolver, which basically just swaps the file name to its
full name when the file name includes one of aliases.

I also separated Parcel configuration that should be used for library
build from the application build. So we can scope down the impact only
to library build. Currently, the library build configuration extends the
application configuration, and adds alias resolver on top of it.

In addition to these changes, the library build command is included in
the `stage` command. When the library build fails, the preview build
will also fail.

### Things I wish I could figure out better
#### Resolver chain?
I want alias resolver to resolves only the alias, and leave the other
stuffs (absolute path or module name resolution) to Parcel default
resolver. For example, let's say a syntax like below was used

```
import Panel from '$components/Panel'
```
I want alias-resolver to translate this into 

```
import Panel from '/app/scripts/components/Panel'
```
and leave it up to further translating this syntax into either
```
import Panel from '/app/scripts/components/Panel.tsx'
```

or 
```
import Panel from '/app/scripts/components/Panel/index.tsx'
```
(There can be even more variety such as
'./app/scripts/components/Panel/index.jsx',
'./app/scripts/components/Panel/index.ts' ...) However, it seems like
Resolver has to return the absolute path for the file or null to pass it
to the next resolver
(https://parceljs.org/plugin-system/resolver/#relevant-api). So I
included a logic to tell if imported file is module or file in
alias-resolver.

#### Update other Parcel-related packages

I tried, but I also faced bunch of other problems that I feel like I
would need another day to figure out. So here, I only ditched parecel's
experimental bundler, which seems to be included in the higher version
by default. The context for experimental bundler is in this PR:
#306


### We need to check

I only checked the library suceeds at being built. We need to check if
the built assets from this branch are the same from the ones of main
branch.
@hanbyul-here
Copy link
Collaborator

I found another problem when I tried to build the UI from ghg-config side. The build fails


@parcel/transformer-sass: Can't find stylesheet to import.
  ╷
1 │ ┌ @use 'uswds-core' with (
2 │ │     $utilities-use-important: true,
3 │ │     $theme-show-notifications: false
4 │ │ );
  │ └─^
  ╵
  .veda/ui/app/scripts/styles/_uswds-theme.scss 1:1  @forward
  .veda/ui/app/scripts/styles/styles.scss 1:1        root stylesheet

  Error: Can't find stylesheet to import.
    ╷
  1 │ ┌ @use 'uswds-core' with (
  2 │ │     $utilities-use-important: true,
  3 │ │     $theme-show-notifications: false
  4 │ │ );
    │ └─^
    ╵
    .veda/ui/app/scripts/styles/_uswds-theme.scss 1:1  @forward
    .veda/ui/app/scripts/styles/styles.scss 1:1        root stylesheet

I think it is the path problem, and should be fixable. I will update once I make more progress.

@hanbyul-here hanbyul-here force-pushed the base-setup-for-react-uswds branch from 0127d9b to 6313135 Compare July 11, 2024 18:15
@hanbyul-here
Copy link
Collaborator

hanbyul-here commented Jul 11, 2024

I think now the build for library, instance, and UI dev environment should all work. phew 💦 I will give an approval, but please test for all the builds before the merge !

@hanbyul-here hanbyul-here force-pushed the base-setup-for-react-uswds branch from 6313135 to a6b1ff9 Compare July 11, 2024 18:19
@dzole0311
Copy link
Collaborator Author

dzole0311 commented Jul 12, 2024

Tested the following:

✅ veda-ui: serving the app, building the app, building the lib, the uswds design system works as before
✅ veda-config: updating the submodule, building and running the instance: NASA-IMPACT/veda-config#417
✅ veda-config-ghg: updating the submodule, building and running the instance US-GHG-Center/veda-config-ghg#438
✅ veda-config-eis: updating the submodule, building and running the instance NASA-IMPACT/veda-config-eic#123

@dzole0311 dzole0311 merged commit 6029236 into main Jul 12, 2024
8 checks passed
@dzole0311 dzole0311 deleted the base-setup-for-react-uswds branch July 12, 2024 08:25
sandrahoang686 added a commit that referenced this pull request Jul 17, 2024
## 🎉 Features
* 🦗 

## 🚀 Improvements
- Base setup for using react-uswds in the veda-ui
#1029
- Consolidate utils/veda-data(-no-faux-module) methods into new dir and
remove faux module dependencies for Library build
#1030
- Upgrade to Parcel 2.12 and make library build work
#1045
- Update E&A Date Picker to react-calendar with USWDS tokens
#1048

## 🐛 Fixes
- Sort catalog items alphabetically in the E&A modal and the data
catalog #1039
@hanbyul-here hanbyul-here mentioned this pull request Jul 18, 2024
1 task
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.

3 participants