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

Switch to preconstruct for packages dev and build #472

Merged
merged 12 commits into from
Feb 8, 2023
Merged

Conversation

alecdwm
Copy link
Member

@alecdwm alecdwm commented Jan 30, 2023

perfect

@alecdwm alecdwm force-pushed the feat/preconstruct branch 3 times, most recently from f1f6566 to 873cc7f Compare February 1, 2023 00:21
@alecdwm
Copy link
Member Author

alecdwm commented Feb 1, 2023

Some notes so far on the conversion:

  • converted the balances-demo from CRA to vite with tailwind, there's nothing important going on in the diffs inside that directory
  • had to upgrade rxjs to the same version as used inside polkadot.js/api to fix a type error
  • needed to move our config packages into a separate directory so that preconstruct doesn't try (and fail!) to compile them with babel
  • needed to add @babel/plugin-proposal-decorators to our babel config and apply it to the monorepo packages so that they could be compiled by babel (previously they were compiled by tsc)
  • talisman-ui wasn't compiling and from the error it had it looked like it was struggling to parse the classNames function, so I just moved that into our util package.
    but I think the underlying cause for the error was actually the svg parsing, which I had to then fix afterwards anyway.
    I left the classNames change in because I think it makes sense for it to be in the common util package anyway
  • i had to fork preconstruct in order to get it to compile the svg files in talisman-ui. the issue to track the upstreaming of my changes is here
  • some packages were missing deps in their package.json, preconstruct throws errors when this happens so I added them in
  • require is not bundled by preconstruct, so I had to change the loggers to import their package.json names instead
  • preconstruct doesn't let us build individual packages, only the whole thing as a collection - so we can't take advantage of turborepo's caching on an individual package basis

@alecdwm alecdwm marked this pull request as ready for review February 1, 2023 08:23
@alecdwm alecdwm requested review from 0xKheops and chidg February 1, 2023 08:24
@alecdwm alecdwm changed the title Switch to preconstruct for dev Switch to preconstruct for packages dev and build Feb 1, 2023
0xKheops
0xKheops previously approved these changes Feb 1, 2023
Copy link
Contributor

@0xKheops 0xKheops left a comment

Choose a reason for hiding this comment

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

LGTM! top tier PR!

@alecdwm
Copy link
Member Author

alecdwm commented Feb 2, 2023

Pushed one more change, I realised I could avoid doing those global declares by importing from @talismn/balances/plugins instead of ../plugins inside the @talismn/balances package.
And doing so actually reduces the number of lines changed by the PR! 😄

@alecdwm
Copy link
Member Author

alecdwm commented Feb 2, 2023

and now rebased off of latest dev

@alecdwm alecdwm self-assigned this Feb 7, 2023
@alecdwm alecdwm added the enhancement New feature or request label Feb 7, 2023
Copy link
Contributor

@chidg chidg left a comment

Choose a reason for hiding this comment

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

LGTM

@alecdwm alecdwm merged commit 8adc7f0 into dev Feb 8, 2023
@alecdwm alecdwm deleted the feat/preconstruct branch February 8, 2023 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants