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.

Sign up for GitHub

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

Jump to bottom

Add wrangler plugin #572

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Add wrangler plugin #572

merged 5 commits into from
Mar 26, 2024

Conversation

DaniFoldi
Copy link
Contributor

Hi 👋,

This PR aims to implement a plugin for Cloudflare Workers, or more specifically their wrangler cli. The config file is wrangler.toml with json config in experimental status (has been for quite a while), and contains one relevant entry, the main entrypoint of the worker.

I think I managed to add a correctly minimal fixture this time, the new config api made it even simpler compared to the Astro one 🎉, great work!

webpro
webpro approved these changes Mar 25, 2024
View reviewed changes
Copy link
Collaborator

@webpro webpro left a comment

Choose a reason for hiding this comment

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

Thanks! Seems like some smooth sailing, just a few minor notes and I'll be happy to merge it

packages/knip/test/plugins/wrangler.test.ts Outdated
cwd,
});

// console.log(issues);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit

packages/knip/src/plugins/index.ts Outdated
@@ -54,3 +54,4 @@ export { default as vue } from './vue/index.js';
export { default as webpack } from './webpack/index.js';
export { default as wireit } from './wireit/index.js';
export { default as yorkie } from './yorkie/index.js';
export { default as wrangler } from './wrangler/index.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move it up so it's ordered alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit

packages/knip/src/ConfigurationValidator.ts Outdated
@@ -77,6 +77,7 @@ export const pluginSchema = z.union([
]);

const pluginsSchema = z.object({
wrangler: pluginSchema,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move it down so it's ordered alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit

@webpro
Copy link
Collaborator

webpro commented Mar 25, 2024

Please also make sure the tests pass

@DaniFoldi
Copy link
Contributor Author

Hi,

Thanks for the quick review! I'm actually not sure why the test is failing, at first it didn't fail locally, but now it is so I'm not sure why that was. I'm not sure what the right way to fix the test is, actually, shouldn't wrangler being the enabler mark it as used? Or should I add a script to package.json that references it?

@webpro
Copy link
Collaborator

webpro commented Mar 25, 2024

shouldn't wrangler being the enabler mark it as used? Or should I add a script to package.json that references it?

Usually it's best to have it close to a real-world scenario, so both sounds good :)

@DaniFoldi
Copy link
Contributor Author

Well I gave it a go 😅, now it's complaining about wrangler being unused and unlisted at the same time, I'm quite tired so I'll probably fix it tomorrow:

    +   binaries: 1,
    -   binaries: 0,
        classMembers: 0,
    +   dependencies: 1,
    -   dependencies: 0,
    ```

@webpro
Copy link
Collaborator

webpro commented Mar 26, 2024

Knip checks the package.json#bin field of the package in node_modules, so I just added that :)

@webpro webpro merged commit 82c84be into webpro-nl:main Mar 26, 2024
11 checks passed
@webpro
Copy link
Collaborator

webpro commented Mar 26, 2024

🚀 This pull request is included in v5.6.0. See Release 5.6.0 for release notes.

@webpro
Copy link
Collaborator

webpro commented Mar 26, 2024

Thanks again, Dániel!

@DaniFoldi
Copy link
Contributor Author

Makes sense, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webpro webpro webpro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DaniFoldi @webpro