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

feat: support for multiple paths in in source configuration #230

Merged
merged 20 commits into from
Dec 6, 2022

Conversation

jackiewmacharia
Copy link
Contributor

@jackiewmacharia jackiewmacharia commented Nov 30, 2022

🎉 Thanks for sending this pull request! 🎉

https://github.com/netlify/pod-compute/issues/295

This PR adds support for multiple paths in ISC. It allows for path to be either a string or array of strings.

Checklist

Please add a x inside each checkbox:

  • I have read the contribution guidelines.
  • The status checks are successful (continuous integration). Those can be seen below.

A picture of a cute animal (not mandatory but encouraged)
cute animal

@jackiewmacharia jackiewmacharia requested a review from a team November 30, 2022 12:35
node/declaration.ts Outdated Show resolved Hide resolved
node/declaration.test.ts Outdated Show resolved Hide resolved
node/declaration.test.ts Show resolved Hide resolved
node/declaration.ts Outdated Show resolved Hide resolved
node/declaration.ts Outdated Show resolved Hide resolved
declarations.push({ ...declaration, ...config, path })
})
// if empty path array with cache set, add declaration with cache
else if (config.cache) declarations.push({ ...declaration, cache: config.cache })
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why we're addressing the cache property specifically here. What happens if we add more configuration properties? Will they have to be added to these conditions?

Copy link
Contributor

Choose a reason for hiding this comment

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

@eduardoboucas I made a change that will include any type of config, I think it had to do with the overall conditional in the function. Let me know if it doesn't make sense?

node/declaration.ts Outdated Show resolved Hide resolved
@khendrikse khendrikse self-assigned this Dec 5, 2022
@khendrikse khendrikse force-pushed the feat/support-multiple-paths-in-isc branch from c1ae79c to cbf007d Compare December 5, 2022 13:57
node/declaration.ts Outdated Show resolved Hide resolved
node/declaration.ts Show resolved Hide resolved
node/declaration.test.ts Show resolved Hide resolved
@khendrikse
Copy link
Contributor

@eduardoboucas sorry all my comments weren't sent because I did not review correctly :p

node/declaration.test.ts Outdated Show resolved Hide resolved
node/declaration.test.ts Outdated Show resolved Hide resolved
node/declaration.test.ts Outdated Show resolved Hide resolved
node/declaration.test.ts Outdated Show resolved Hide resolved
node/declaration.ts Outdated Show resolved Hide resolved
node/declaration.ts Show resolved Hide resolved
node/declaration.ts Show resolved Hide resolved
node/declaration.ts Show resolved Hide resolved
node/declaration.ts Outdated Show resolved Hide resolved
node/declaration.ts Outdated Show resolved Hide resolved
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

✨ Awesome work, Jackie and Karin! ❤️

@khendrikse khendrikse merged commit d4ec906 into main Dec 6, 2022
@khendrikse khendrikse deleted the feat/support-multiple-paths-in-isc branch December 6, 2022 13:53
Skn0tt pushed a commit to netlify/build that referenced this pull request Apr 23, 2024
…edge-bundler#230)

* feat: support for multiple paths in in source configuration

* fix: use array for isc path properties

* fix: support both array and string path properties

* fix: fix linting

* fix: have a string usecase in declaration tests by transforming json array to string

* fix: remove file-level eslint-disable-max-depth

* fix: change conditional for cache config and path length

* fix: change conditional for adding all different types of config

* fix: remove fallback to empty object

* fix: change comment

* fix: undo changing config fixtures to use an array as path

* fix: update comments

* Update node/declaration.ts

Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>

* fix: change mutation of config.path

* fix: formatting

Co-authored-by: khen <30577427+khendrikse@users.noreply.github.com>
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
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.

None yet

4 participants