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

Add legacy and runContext script sync rule middlewares #909

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nezuo
Copy link
Contributor

@nezuo nezuo commented May 7, 2024

No description provided.

@nezuo
Copy link
Contributor Author

nezuo commented May 7, 2024

I just saw that sync rules don't apply to init. scripts. It would be impossible to make a sync rule that turns scripts into their legacy counterparts with emitLegacyScripts off that works with init. scripts.

@Dekkonot
Copy link
Member

Dekkonot commented May 7, 2024

My thought was that we'd use the existing middleware for Legacy behavior and add middleware for the RunContext variants, since it'd allow changing a single script easier. I'm not married to that idea, but I think it makes more sense than the other way around.

What do you think?

@Dekkonot
Copy link
Member

Dekkonot commented May 7, 2024

I just saw that sync rules don't apply to init. scripts. It would be impossible to make a sync rule that turns scripts into their legacy counterparts with emitLegacyScripts off that works with init. scripts.

This is something we need to address eventually. While implementing syncback, I swapped directories (you can see that here) to be Middleware and it made everything easier. We can probably piggyback off of that when syncback gets merged to support customizing them somehow.

@nezuo
Copy link
Contributor Author

nezuo commented May 8, 2024

My thought was that we'd use the existing middleware for Legacy behavior and add middleware for the RunContext variants, since it'd allow changing a single script easier. I'm not married to that idea, but I think it makes more sense than the other way around.

What do you think?

If we do this, there won't be a way to say "give me a script based on the emitLegacyScripts option." That said, I can't think of a case where that's useful.

I do think this would make defining rules more succinct. For example, with the current solution my PR has:

// Gradually adopt Legacy scripts.
"emitLegacyScripts": false,
"syncRules": [
	{
		"pattern": "*.legacy.server.lua",
		"use": "legacyServerScript",
		"suffix": ".legacy.server.lua"
	},
	{
		"pattern": "*.legacy.client.lua",
		"use": "legacyClientScript",
		"suffix": ".legacy.client.lua"
	},
],
// Gradually adopt RunContext scripts.
"emitLegacyScripts": false,
"syncRules": [
	{
		"pattern": "*.server.lua",
		"use": "legacyServerScript",
		"suffix": ".server.lua"
	},
	{
		"pattern": "*.client.lua",
		"use": "legacyClientScript",
		"suffix": ".client.lua"
	},
	{
		"pattern": "*.rc-server.lua",
		"use": "serverScript",
		"suffix": ".rc-server.lua"
	},
	{
		"pattern": "*.rc-client.lua",
		"use": "clientScript",
		"suffix": ".rc-client.lua"
	}
],

However, with your proposal these would become:

// Gradually adopt Legacy scripts.
"emitLegacyScripts": false,
"syncRules": [
	{
		"pattern": "*.legacy.server.lua",
		"use": "serverScript", // This is identical but it's now serverScript instead of legacyServerScript.
		"suffix": ".legacy.server.lua"
	},
	{
		"pattern": "*.legacy.client.lua",
		"use": "clientScript",
		"suffix": ".legacy.client.lua"
	},
],
// Gradually adopt RunContext scripts.
"emitLegacyScripts": true,
"syncRules": [
	{
		"pattern": "*.rc-server.lua",
		"use": "runContextServerScript",
		"suffix": ".rc-server.lua"
	},
	{
		"pattern": "*.rc-client.lua",
		"use": "runContextClientScript",
		"suffix": ".rc-client.lua"
	},
],

Gradually adopting RunContext scripts now only uses 2 rules instead of 4.

I'm on board with your proposal, do you have an opinion on what the RunContext use values should be called? runContextServerScript?

@Dekkonot
Copy link
Member

Dekkonot commented May 8, 2024

That's a mouthful, but I can't think of anything better at this moment. It might be fine to abbreviate RunContext to rc just for brevity, but I'm also mindful of the fact that rcServerScript is basically the same as serverScript if you aren't paying attention.

@nezuo
Copy link
Contributor Author

nezuo commented May 10, 2024

I'm having a hard time deciding how to update the changelog because it's divided into a table from "use" value to "file extension," but file extension doesn't make sense for these rules anymore.

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

As implemented right now, this makes emitLegacyScripts not function. I imagine this isn't deliberate?

With regards to the changelog, I don't know! The table I made was meant as a quick guide to syncing rules, but I think it's probably fine to have this as its own changelog entry (probably directly below the main sync rules one).

@nezuo
Copy link
Contributor Author

nezuo commented May 14, 2024

I've introduced legacyServerScript and legacyClientScript as a fix for emitLegacyScripts. Now serverScript and clientScript will respect the project's emitLegacyScripts option.

@nezuo nezuo changed the title Add legacy script sync rule middlewares Add legacy and runContext script sync rule middlewares May 14, 2024
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