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

For discussion of issue #198 (dont merge!) #202

Closed

Conversation

goblinfactory
Copy link
Contributor

@goblinfactory goblinfactory commented Feb 15, 2024

In order to add support for including optional IGNORE settings in site.yaml, It is necessary to make some rather large changes to

  • nuefs.js (specifically the critical fswalk)
  • nuekit.js
  • site.js
  • fswalk

... due to the various hidden dependancies surrounging isLegit and the whole configuration merging.

In order to make the changes i've also had to move isLegit out to a separate file and create a factory method to return an "isLegit" function to avoid having to pass the "ignores" with every invocation.

I'm not comfortable that this is the right way to inject global configuration/settings, and that some of the existing bootstrapping might need discussing and re-thinking.

I've created a PR so that these changes can be easily shared and discussed.

Changes to critical fswalk

This PR also shows that allowing user configurations in site.yaml related to files, ends up requiring delicate changes to fswalk, a critical piece of code that we need to limit changes to. (imho)

Hard code support for Cloudflare and leave user configuration for the next major release

Coverage is currently low; therefore for now, I recommend hard coding in support for excluding cloudflare worker functions folder, but leaving out the ability to customise it further with configurations in site.yaml.

I think a change to injecting global configuration settings requires a discussion, and might end up rethinking bootstrapping and be better to wait to include in a next major release? (hard coding solid good defaults for pit-of-sucess and idiot proofing the first version)

Hopefully the code changes in the discussion branch make sense; but it's a fairly chunky bit of changes, so I'll happy answer any questions if that will help.

I think that adding any other user overridable settings (in site.yaml) would end up requiring very similar changes, hence this PR for discussion only.

Your thoughts?
Alan

p.s. I've got all the tests passing here. So while this branch is for discussion, it's passing all the tests.
I had to change the beforeAll and AfterAll to beforeEach and AfterEach, because some tests overwrite the site.yaml, and cause side effects. I though it would slow down the tests, but BUN is just unbelievably fast and didnt slow down at all. Having that nice clean separation for these integration tests is important.

After we've discussed this ... I'll create a new PR for hard coding support for ringfencing (protecting) cloudflare functions folder, and update the tests and bring in the beforeEach and afterEach change into that PR.

@tipiirai
Copy link
Contributor

Nice. I liked it! I was afraid of bloat, but it wasn't there! Not because of you, but it's just constantly everywhere and always.

@goblinfactory
Copy link
Contributor Author

@tipiirai So just to be clear, are you ok with the suggestion to NOT add "ignores" to site.yaml pre ver 1 or 2?
Or does your comment mean you actually like this PR?
So that I'm not blocked by this, I'm going to do the hard coding anyway as a separate PR, and wait for your feedback on this PR. txs, A

@goblinfactory
Copy link
Contributor Author

closing; I still have this branch if we need to discuss. Issue #204 is a good work around.

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