The `Node` superclass setting defaults for its subclass is error prone.

Aberta
  • Algo errado?
#1,978 3 comentários 0 reações 0 responsáveis Ver no GitHub

Mantenedores costumam responder em até 1 dia

Ninguém assumiu esta issue ainda.

Avaliação

Dificuldade
5/5
Tempo estimado
Mais de uma semana
Facilidade para iniciantes
30/100
Tipo de issue
Funcionalidade
Clareza
Razoavelmente clara
Status de atividade
Estagnada
Stack de tecnologia
javascript, typescript
Domínio
web-dev

Direção de pesquisa

Start with the Node constructor in lib/node.js at the linked line, then reproduce the TypeScript subclass example using an ES2022 target and useDefineForClassFields. Define and document a safe default-value behavior for subclasses, with regression coverage showing that values passed to super() are not silently reset.

Escrita pelo modelo de indexação a partir do texto da issue.

Descrição

The constructor for the Node class receives a map of default values, and then assigns the values to itself the following way:

https://github.com/postcss/postcss/blob/1cc6ac3bf43a12460d3396e2cb76f14075daea1b/lib/node.js#L52

This pattern is error prone because in JS, property initialization happens after calling the super() constructor. That means that even if a value is set by calling super(defaults) from a child node, the value could then be silently reset to undefined or some other initial value if the property happens to be declared in the subclass.

Example snippet in typescript:

class MyNode extends Node {
  private _foo!: string;
  get foo() { return _foo }
  set foo(f: string) { this._foo = f };

  constructor(defaults) {
    super(defaults);
  }
}

const myNode = new MyNode({foo: 'foo'});
console.log(myNode.foo); // OUTPUT: undefined

We expected the output to be "foo", but instead it was undefined because the MyNode properties are initialized after calling super(). This is JS behavior according to spec, and it can only be seen when compiling TS using the ES2022 target (or above) and enabling the tsconfig option useDefineForClassFields. The reason TS didn't fail is because TS moved the property initialization prior to calling the constructor, but it fails now that it emits more accurate JS.

The fix for this example using the current PostCSS API in this case is to replace private _foo!: string with declare _foo: string. However, this has two issues. First, it's not ideal to use declare in this case. Second, the error is silent and is only noticeable with thorough testing, which is therefore error prone because it's somewhat easy to miss.

So, I believe the PostCSS API should do something different to keep/set the default values in a different way that doesn't invite the user to write this pattern. Maybe the node super class could just keep an internal copy of the default values inside a JSON property called _defaults, and the getters/setters would just access that?

Linguagem predominante
TypeScript
Estrelas
29k
Forks
1.7k
Merge médio
1d 14h
PRs com merge (30d)
9

Preparar o ambiente

Abrir no Codespaces

Inicia o contêiner de desenvolvimento do projeto no navegador, com a sua própria conta do GitHub.

Primeiros passos

  1. Leia a issue inteira e depois o guia de contribuição do projeto.
  2. Comente na issue dizendo que vai assumir — evita que duas pessoas façam o mesmo trabalho.
  3. Faça um fork do repositório e trabalhe em uma branch.
  4. Abra um pull request que referencie o número da issue.

Mais de postcss/postcss

Todas as issues de postcss/postcss

Issues semelhantes

Mais issues de TypeScript

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.