chore(core): streamline object helpers

[jycouet]

Will simplify in general & reduce the api surface.

To be merged before: #680

[changeset-bot]

🦋 Changeset detected

Latest commit: 5518f9c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
sv	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

npx https://pkg.pr.new/sveltejs/cli/sv@685

npx https://pkg.pr.new/sveltejs/cli/svelte-migrate@685

commit: 5518f9c

[manuel3108]

I'm not sure I fully understand what's happening here? Also, what made you change that?

I do think the cleanest object helper we currently have is the create one:

cli/packages/addons/eslint/index.ts

Lines 110 to 120 in 7848d53

	const svelteTSParserConfig = object.create({
	files: ['**/*.svelte', '**/*.svelte.ts', '**/*.svelte.js'],
	languageOptions: {
	parserOptions: {
	projectService: true,
	extraFileExtensions: ['.svelte'],
	parser: variables.createIdentifier('ts.parser'),
	svelteConfig: variables.createIdentifier('svelteConfig')
	}
	}
	});

It handles nested object's perfectly fine, has helpers for converting bools, arrays and strings to the appropriate ast alternatives and so on. I do understand that the overrideProperties and property are not exactly there yet, but I could imagine that converting those should be quite simple. That would also bring the benefit to unifying with create even further.

But as I said, im not fully understanding why this change is done, so let me know if I missed anything.

[43081j]

its to avoid repeating code like this everywhere:

const kitConfig = config.properties.find(
				(p) => p.type === 'Property' && p.key.type === 'Identifier' && p.key.name === 'kit'
			) as AstTypes.Property | undefined;

// ...
if (kitConfig) {
  // override it
} else {
  // set a default
}

you should be able to use overrideProperty with a value to achieve the same job

but then when you go beyond one level of nesting, the current overrideProperty didn't support that. so you'd end up repeating this pattern:

if (kitConfig) {
  const deepObj = kitConfig.properties.find(...);
  if (deepObj) {
    const deeperObj = deepObj.properties.find(...);
    // ...
  } else {
    // ...
  }
} else {
  // ...
}

it'd be cleaner to be able to:

overrideProperty(config, {
  path: ['kit', 'deepObj', 'deeperObj'],
  value: whatever
});

which entirely replaces the big block of if/else chains

[manuel3108]

Yeah, that totally makes sense, but isn't that what overrideProperty was already doing? Im probably still missing something

[jycouet]

Yeah, that totally makes sense, but isn't that what overrideProperty was already doing? Im probably still missing something

Yes, but not in a nested way. now you have path | name

[43081j]

for example, you had to do this (pseudo code):

const kit = object.property(obj, {
  name: 'kit',
  fallback: {}
});

const deepObj = object.property(kit, {
  name: 'deepObj',
  fallback: {}
});

const deepestObj = object.overrideProperty(deepObj, {
  name: 'deepestObj',
  value: 303
});

to produce:

{
  kit: {
    deepObj: {
      deepestObj: 303
    }
  }
}

but now you can:

object.overrideProperty(obj, {
  path: ['kit', 'deepObj', 'deepestObj'],
  value: 303
});

[manuel3108]

Thank you, I did finally understand it. Now my last question is if we are able to and want to align it further with the object.create syntax. The syntax is as follows:

const arrayOfFancyAstNodes = array.create();
const bla = object.create({
	foo: {
		bar: 'test',
		baz: arrayOfFancyAstNodes 
	},
	test: true
});

Your last example would look like this:

object.overrideProperty(obj, {
  kit: {
    deepObj: {
      deepesteObj: 303
    }
  }
});

IMO, this would make it even simpler and align with the create syntax even more. We could probably even share some code with object.create. I don't know why I didn't do this when I was reworking the api in #597. Im happy to give this a try if you agree

[43081j]

so the difference would be that we pass an object as input?

the implementation of overrideProperty then has to loop through properties of that input object deeply, doing what it does in this branch?

basically:

for (const prop of inputObj) {
  if (isObject(inputObj[prop])) {
    // recursively do this overall algo
  } else {
    overrideProperty(originalObj, {property: prop, value: inputObj[prop]});
  }
}

if i understood correctly, that makes sense to me 👍

[jycouet]

When I have:

const obj = {
  hello: 'world',
  kit: {}
}

and then do

object.overrideProperty(obj, {
  kit: {
    deepObj: {
      deepesteObj: 303
    }
  }
});

I'm a bit worried that hello: 'world' will be gone (I know that it will not). But the API feel liek it will be gone.
So, it's maybe better to not have the same way as create to convay another intent.

In the end, I don't really mind, if you prefer the object you can do 👍, I just gave my 2 cents :D

[manuel3108]

Ok, I have made the changes I suggested earlier. I think it turned out great, but let me know what you think.

Also i removed the export for object.overrideProperty as it had only a few usages and basically the same problem as discussed above.

[manuel3108]

This is the only thing, I'm not super happy about. For one, I don't like that this is just a third argument as a function, should probably be an object.

Additionally this is now executed for every property along the path, that's why the additional checks are needed.

[jycouet]

using object.create like syntax is maybe not that bad.

// array style
[ { name: 'id', value: '007' } ]
// object.create syntax (nice & short)
{ id: '007' }
// alternative (shorter, I like, but maybe less readable)
[ ['id', '007'] ]

third argument as a function, should probably be an object

As it's optional, and not used a lot I think that this is ok.

this is now executed for every property along the path

This is more problematic IMO. What about { a: { b: { a: true } } }, we will have 2 key.name of a.

I'm not sure I have a lot of proposal. Maybe we could have a transformProperty taking also the same api, but with a selector of where to do the transform?

if(adapter.package !== '@sveltejs/adapter-auto'){
object.transformProperty(
  config,
  { kit: { adapter: true } }, // selector ?
  (property) => {
      property.leadingComments = [];
      return property;
    }
  );
}

Or the previous way ? Or the previous alternative way ?

[manuel3108]

I do think that could work. But is it worth the effort?
We could also only run that function on leaf nodes, but then we would need to think about a better naming for this.

[jycouet]

I though about leaf nodes but:

{
  b: { a: true }, 
  c: { a: '1' } 
} 

Both are leaf, and both are 'a'.
Not easy 😅

[manuel3108]

okay, you got me

Any reason you decided against something like that in your last example? I think that would make it even more clear. I did not check if can convince typescript, but it should work just fine

if(adapter.package !== '@sveltejs/adapter-auto'){
  object.transformProperty(
    config,
    { kit: { adapter: (property) => { /* ... */ } } }
  );
}

[43081j]

not to throw a spanner in the works - but we could also not do it 😅

i'd have to read through the codebase again, but iirc we don't need this often. we use it to clear leading comments usually

so you could just do this at the callsite:

object.overrideProperty(config, {
  kit: {
    adapter: {
      foo: 303
    }
  }
});

// probably turn this into a helper or make property accept a path
object.property(
  object.property(config, 'kit'),
  'adapter'
).leadingComments = [];

[jycouet]

The question is: "Why do I like these?!" 😅

I added a few more tests to cover some more usecases.
Starting to look good. No?!

Let me know

[jycouet]

Acutally,

object.overrideProperty(config, {
  kit: {
    adapter: {
      foo: 303
    }
  }
});

Will already reset leading comments (as it's done today, maybe it could be better?!)

And it's not what we want here.

I see: object.overrideProperty as the nice and easy API
and object.transformProperty where you can do everything.

---

But looking at your comment, I like the simplicity! :o

Going out a bit...

[43081j]

are you sure? overrideProperty will replace the value (of foo in this case), but not the property node itself as far as i remember

this code was previously clearing the leading comments of the adapter property, and setting the value to a new function

but we rarely do this so i think it makes more sense to keep overrideProperty simple and just clear the property's leading comments separately

[jycouet]

I added test for overrideProperty and yes you are right, it's only the value.

I removed transformProperty as it was for a very specific case, and added a small helper propertyNode that will do the nice adapter thing.

//Happy

[manuel3108]

Thanks!