Setup CI

[rodion-arr]

No description provided.

[rodion-arr]

@wesleytodd Node v10 is not supported?

[wesleytodd]

Hey, that was not intentional. It looks like the little helper package I wrote to make using filesystem fixtures more simple is broken in node@10. https://github.com/wesleytodd/fs-test-fixtures/actions/runs/184565719

I will take a look at why and publish a new version.

[wesleytodd]

I think github started disabling these sort of nightly runs. What value do you see in them when we also run on PR and pushes to main?

[rodion-arr]

We are using this setup in wiby and I assumed this can be also applied here.

[wesleytodd]

I would love to hear the reasoning behind the decision. In projects which are not active daily from multiple contributors I do not usually see the point in a nightly build. I guess I just think of a nightly as a way to integration test many in-flight features from different engineers, so without that it seems better to just test each commit on a PR branch and main.

[rodion-arr]

I removed it for now and maybe we can kindly summon @dominykas here to add more thoughts regarding night scheduler.

[wesleytodd]

IMO, a lint failure is a test failure. Is there a reason not to also fail on lint errors and so just run npm t in all of them? I have in the past seen the lint configuration fail in a certain version of node, and if that is the case I think we would want to know.

[rodion-arr]

I don't really have strong opinion here, from my side both variants are OK, so if you'd like to remove separate lint job and run lint on every node version, please let me know.

[wesleytodd]

It keeps it simple, but I would be interested in hearing others opinions on the topic. Lets not consider this a blocker, and if no one shares an opinion we can merge and revisit later.

[rodion-arr]

Hey, that was not intentional. It looks like the little helper package I wrote to make using filesystem fixtures more simple is broken in node@10. https://github.com/wesleytodd/fs-test-fixtures/actions/runs/184565719

I will take a look at why and publish a new version.

Submitted PR with fix - wesleytodd/fs-test-fixtures#1

[rodion-arr]

One more place existed in tests using Object.fromEntries which not present in v10, so I replaced it with shim as dev dependency.

[wesleytodd]

@dominykas Should I be integrating the managed test workflow here? Is there a way to use that without engines? I just have not touched this in a while, and I don't think I am ready to add engines until we have a discussion about it in this context.

[dominykas]

Is there a way to use that without engines?

No, there's no such way. Not sure it even makes sense without engines, TBH - you need to give it a minimum supported version (or else you're supporting all versions?)

[wesleytodd]

I was thinking it would just default to current lts lines. I expressed this in the thread in that repo, but my main concern is just that it makes adopting this CI workflow technically a breaking change if packages didn't have it (like this one) initially.

[dominykas]

technically a breaking change if packages didn't have it (like this one) initially.

There is an implicit version regardless? This package does not support all Node.js versions as it is and it never did - it was always testing in a specific set, didn't it? So adding node >= 10 into engines is not "breaking" per se? Unless you see the addition of the engines into the package.json as breaking itself, but that would probably only break people who were using unsupported versions anyways?

[ljharb]

When there’s no engines specified, the package supports whatever it happens to work in at any point in the major line, and excluding any of those when adding an engines field is what’s breaking. Your original intention is irrelevant, only what it actually works matters.

[wesleytodd]

Yeah, I consider the addition of engines alone the breaking change in any package. I mean maybe I just need to bite the bullet on it and make the change, but right now I believe this code runs back quite a few node versions and I don't have any other code related reason to break that.

[dominykas]

🤔 would it make sense to run in all versions if engines is undefined? (just thinking out loud for now, unsure if it's a good idea just yet)

But I mean, we still need to define some minimum version to run in? Even if it's 0.1?

create-pkg itself will probably still be limited by what npm versions it supports and what Node versions these npm version support?

[wesleytodd]

would it make sense to run in all versions if engines is undefined?

I think either this or current lts.

But I mean, we still need to define some minimum version to run in? Even if it's 0.1?

This is why I prefer lts over "all", I think that the "I dont care just get me publishing" author is better off with LTS than making decisions on a min version.

will probably still be limited by what npm versions it supports and what Node versions these npm version support?

Oh yeah, like as soon as we go 1.0 I would say that this drop all but latest lts and whatever versions of npm are supported at the time.

[ljharb]

Yes, if engines is missing, then the implicit default is *, so definitely NOT just LTS.

[wesleytodd]

Technically correct is not helpful for users here. I understand that it is implicitly all (*) but that is never what a real user would want, right? IMO it would be better to choose the ergonomic path, and then recommend (maybe by printing warnings that engines is missing) that they explicitly set engines with a well defined support range.

[ljharb]

It's definitely sometimes what I want, as a real user.

We can use whatever default in the action we like, but I don't think it's helpful for users to encourage omission of an explicit engines field - that's just a breaking change they'll have to accept.

[wesleytodd]

It's definitely sometimes what I want, as a real user.

You are unique here lol. But even in that case I would say you can just do * and get all. What we are talking about here is for the folks who don't know and maybe are just trying to get their first JS package setup. It is alright if they get a warning about adding engines and then tests which run against lts_latest versions only IMO.

[ljharb]

I think their CI should fail until they add engines, even if the testing range is explicitly specified. Omitting it is dangerous and harmful and we shouldn't enable folks to continue to do so, whether new users or experienced ones.

[dominykas]

We could check the date of when the workflow file was created and use an oldest lts on that date as a minimum version, with lts_latest as an update policy in the absense of engines 🤔 Not sure how feasible the implementation is, and yes, I understand what the edge cases are, but maybe it's a "good enough" compromise?

My personal preference is to not support the missing engines, though. I'd even prefer to not have an setting for the action to define that minimum version, because engines is useful metadata on its own and if you support two ways of defining minimum versions, then all of a sudden you also have to implement, explain and document which one takes precedences, and I can guarantee that there will be people who use both, have them different and wonder what's happening. So purely from API design surface, I think having just one way of achieving this is simpler, even if it does require some people to explicitly add an engines field.

I do not consider adding it a breaking change, if it does not change the supported version - I feel it's more like a piece of clarifying documentation in that case, because it only defines the expectations in one more place; someone having different expectations to what the author provided is not a breaking change on its own.

[ljharb]

Again, it is only a breaking change if a version that worked at any point in the major line, is excluded by engines.

[wesleytodd]

Ok, sounds like I am in the minority on this opinion. Honestly I forgot we were having this conversation in this repo and not the action one (where we were having a parallel convo on the same topic). As it applies to this repo this conversation is moot, as the package is unused/new. I will post a synopses of this discussion in the actions issue and then also close that one.