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 tsup with imported json #405

Closed
wants to merge 1 commit into from

Conversation

ashleyfrieze
Copy link
Contributor

@vscheuber - this is a deeper change

  • Upgrade Node to v18
  • Change build completely from babel to tsup
  • Import all json files in the "production" code. I note that the MockForgeRockApi still uses import.meta.env and readFileSync and I'm ignoring that
  • Modify the build in the pipeline

I'm not sure how precisely to test that the new package is fit for consumers. This may be something that you know better how to test. However, I think this version of the code should operate a lot better for packaging in binary with FrodoCLI.

We should also note that the typescript code comes up with a lot of "compile" errors - things which really don't follow the prescribed types. This isn't something I've introduced, but represents general risks around the code. Worth looking at longer-term.

@ashleyfrieze ashleyfrieze force-pushed the switch-to-tsup branch 3 times, most recently from 39e1080 to 8086326 Compare May 2, 2024 12:12
@vscheuber
Copy link
Contributor

@ashleyfrieze , thank you so much for this PR! @hfranklin and his team are working on the bundling. tsup looks really promising but I believe they are going down the esbuild path.

@ashleyfrieze
Copy link
Contributor Author

@vscheuber - I agreed to do this with @hfranklin on the last call. tsup is a superset of esbuild and I firmly believe this approach will work really well with the frodo cli changes.

@hfranklin
Copy link
Collaborator

@ashleyfrieze I split your commit into two, one with the changes to remove import meta and the other for tsup. The commits are on two branches (remove-import-meta-url and switch-to-tsup) in the TriVir fork of this repo. Do you want to pull the changes into your repo and create the PR for remove-import-meta-url or do you want me to go ahead and do it?

@ashleyfrieze
Copy link
Contributor Author

Hey @hfranklin - please go ahead and do it the way you'd prefer to. If it works for you, then I'm delighted.

My next step will be to try to bump axios when this is done. Hopefully the work I did has been helpful!

@hfranklin
Copy link
Collaborator

@ashleyfrieze It was very helpful. I split into two changes so we can complete the import meta changes before changing the build.

@vscheuber
Copy link
Contributor

merged with additional changes in pr/405 (#412). Thank you, @ashleyfrieze, for getting us on the right track! Your PR was extremely helpful!

@vscheuber vscheuber closed this Jun 11, 2024
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