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

chore: build plugins with package #61

Open
wants to merge 2 commits into
base: assemble-plugins
Choose a base branch
from

Conversation

jarredkenny
Copy link

Not sure if this is how you planned on packaging plugins, but this at the very least has allowed me to link the package locally and import the Maestro plugin via:

import { Maestro } from "@ltipton/parkin/build/esm/plugins";

@lancetipton
Copy link
Owner

lancetipton commented Sep 11, 2023

Hey @jarredkenny - I honestly hadn't put much thought into it. A plugins folder just seemed like a good approach when I first did it.

Ideally, It would be best to setup an interface for accessing the Parkin internals, as this Maestro integration feels more like a separate plugin then part of the core repo.

That said, it would take a bit to think through how it should work and to actually implement it. So in the mean time, I think how you have done it is fine for now.

Unfortunately, the CI seems to be failing on an odd error.
I don't have time to look into it today, but I should have time to take a look tomorrow.

Assuming we can get the CI fixed, I'm happy to merge this.

Copy link
Owner

@lancetipton lancetipton left a comment

Choose a reason for hiding this comment

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

^

@jarredkenny
Copy link
Author

The CI error seems like mixing CJS and ESM, not seeing this when building locally so perhaps related to node version.

I don't want to pile too much on a PR, but you may benefit from moving from esbuild configs to tsup for shipping commonjs and ESM side by side.

https://tsup.egoist.dev/

Looks like my editor wasn't picking up your prettier config either, I didn't realize it had reformatted some things.

@jarredkenny
Copy link
Author

Quick demo of what that might look like here: https://github.com/lancetipton/parkin/pull/62/files

@lancetipton
Copy link
Owner

lancetipton commented Sep 12, 2023

Can you update the .github/workflows/pr-checks.yml file to use this node version 20.5.1
Basically lines 21-24 would look like this:

      - name: Install Node.js
        uses: actions/setup-node@v3
        with:
          node-version: 20.5.1

@jarredkenny - I cloned your branch, and updated the node version just to test it out. Looks like that fixed it. So it's definitely an issue 20.6.0 => https://github.com/lancetipton/parkin/actions/runs/6166099853/job/16735027971?pr=63

  • With your other PR, switching to tsup looks to fix it as well, so thats probably a better option. I just need to work out a few issues with publishing updates.

  • So if you want to hold off on updating the workflow file, I'm good with that, and we can just merge this branch in. Let me know what you would prefer?

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.

2 participants