Skip to content

Commit

Permalink
fix(react): remove attribute when is "false" (#190)
Browse files Browse the repository at this point in the history
  • Loading branch information
igorwessel authored Jul 25, 2023
1 parent 5332b4e commit 373d16e
Show file tree
Hide file tree
Showing 7 changed files with 212 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Meta } from '@storybook/addon-docs'

<Meta title="Docs/Architecture Decision Records/ADR 0008: Why do we need Grids" />

# Why do we need Grids
# ADR 0008: Why do we need Grids

🗓️ 2023-05 · ✍️ [@mauriciomutte](https://twitter.com/mauriciomutte) and [@felipefialho](https://twitter.com/felipefialho_)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { Meta } from '@storybook/addon-docs'

<Meta title="Docs/Architecture Decision Records/ADR 0010: Why do we need a fix in React?" />

# ADR 0010: Why do we need a fix in React?

🗓️ 2023-07 · ✍️ [@igorwessel](https://twitter.com/igor_wessel)

## Context

In order to achieve a better DX with boolean props in React.

## Problems

Today (07/19/2023) the Stencil does not follow the HTML spec for boolean props, which caused problems in testing/styling because expected to not have a attribute in HTML when is false.

Example:

```jsx

<AtomButton disabled={false} />

// output in html

<atom-button disabled="false" />
```

So it affects styles, and tests where the boolean attribute shouldn't exist

Examples:

```css
// regardless if disabled is false/true will still be applied.
atom-button[disabled] {
background-color: red;
}
```

```js
// regardless if disabled is false/true will throw a false-positive because expects the attribute does not exist.
expect(atom - button).toBeDisabled()
```

## Decision

After we consider the options:

- Living with the problem for possibly in the near future react will offer better [support](https://github.com/facebook/react/issues/11347) for web-components.
> We can see [tests in a experimental branch here](https://custom-elements-everywhere.com/libraries/react-experimental/results/results.html)
- We are already transpiling the web-component to React, could use a workaround.

Based on these options, we decide to use workaround which will basically set/remove attributes after [occurs a update in component.](https://react.dev/reference/react/Component#componentdidupdate)
1 change: 1 addition & 0 deletions packages/core/jest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module.exports = {
coveragePathIgnorePatterns: ['/node_modules/'],
collectCoverageFrom: [
'src/components/**/*.{js,jsx,ts,tsx}',
'output-target/**/*.{js,jsx,ts,tsx}',
'!src/**/stories/**',
'!src/**/*.mock.ts',
'!src/**/*.spec.ts',
Expand Down
64 changes: 64 additions & 0 deletions packages/core/output-target/react-boolean.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import {
Compiler,
CompilerSystem,
createCompiler,
loadConfig,
} from '@stencil/core/compiler'
import { createNodeLogger, createNodeSys } from '@stencil/core/sys/node'
import { reactOutputTarget } from '@stencil/react-output-target'
import fs from 'fs'
import { reactBooleanFixOutputTarget } from './react-boolean'

describe('React Boolean Fix', () => {
let compiler: Compiler
let sys: CompilerSystem

beforeAll(async () => {
// https://stenciljs.com/docs/compiler-api#createcompiler
const logger = createNodeLogger({ process })
sys = createNodeSys({ process })

const validated = await loadConfig({
logger,
sys,
config: {
_isTesting: true,
outputTargets: [
reactOutputTarget({
// We are informing a file that does not exist to create the react-library inside that folder.
proxiesFile: './output-target/index.ts',
}),
reactBooleanFixOutputTarget({
attachPropsFile: './react-component-lib/utils/attachProps.ts',
}),
],
},
})

compiler = await createCompiler(validated.config)
})

afterAll(async () => {
await fs.promises.rm('./output-target/react-component-lib', {
recursive: true,
})
await compiler.destroy()
})

it('should apply fix correctly in react library', async () => {
jest
.spyOn(sys, 'writeFile')
.mockImplementation(jest.fn().mockResolvedValue(true))

const fsSpyOn = jest
.spyOn(fs.promises, 'writeFile')
.mockImplementation(jest.fn().mockResolvedValue(true))

await compiler.build()

expect(fsSpyOn).toHaveBeenCalledWith(
expect.any(String),
expect.stringContaining("if (propType === 'boolean')")
)
})
})
79 changes: 79 additions & 0 deletions packages/core/output-target/react-boolean.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/**
* You can read about the need for this file at ADR.
* Link: https://juntossomosmais.github.io/atomium/?path=/docs/docs-architecture-decision-records-adr-0010-why-do-we-need-react-fix--docs
*/

import {
BuildCtx,
CompilerCtx,
Config,
OutputTargetCustom,
} from '@stencil/core/internal'
import fs from 'fs'
import path from 'path'

interface IReactBooleanOutputTargetOptions {
attachPropsFile: string
}

const messages = {
start: 'Generating fix for react boolean',
finish: 'Generate fix for react boolean finished',
}

const runReactBooleanFixOutputTarget = async (
outputTarget: IReactBooleanOutputTargetOptions
) => {
const attachPropsFilePath = path.resolve(
__dirname,
outputTarget.attachPropsFile
)

let attachPropsContent = await fs.promises.readFile(attachPropsFilePath, {
encoding: 'utf-8',
})

attachPropsContent = attachPropsContent.replace(
"if (propType === 'string') {",
`if (propType === 'boolean') {
if (newProps[name] === true) {
node.setAttribute(camelToDashCase(name), '');
} else {
node.removeAttribute(camelToDashCase(name));
}
} else if (propType === 'string') {`
)

await fs.promises.writeFile(attachPropsFilePath, attachPropsContent)
}

export const reactBooleanFixOutputTarget = (
outputTarget: IReactBooleanOutputTargetOptions
): OutputTargetCustom => ({
type: 'custom',
name: 'react-boolean-fix',
generator: async (
_config: Config,
compilerCtx: CompilerCtx,
buildCtx: BuildCtx
) => {
const timespan = buildCtx.createTimeSpan(messages.start, true)

await new Promise((resolve) => {
compilerCtx.events.on('buildLog', (log) => {
const isFinishedTranspileReact =
log.messages.findIndex((message) =>
message.includes('generate react-library finished')
) !== -1

if (isFinishedTranspileReact) {
resolve(true)
}
})
})

await runReactBooleanFixOutputTarget(outputTarget)

timespan.finish(messages.finish)
},
})
17 changes: 10 additions & 7 deletions packages/core/sonar-project.properties
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,23 @@ sonar.test.inclusions=\
src/**/*.spec.js,\
src/**/*.spec.jsx,\
src/**/*.spec.ts,\
src/**/*.spec.tsx
src/**/*.spec.tsx,\
output-target/*.spec.ts
sonar.coverage.exclusions=\
src/components/grid/col/col.tsx
src/components/grid/col/col.tsx\
output-target/*.spec.ts,\
sonar.cpd.exclusions=\
src/components/textarea/**
sonar.exclusions=\
node_modules/**,\
**/dist/**,\
.eslintrc.js,\
jest.config.js,\
src/*.spec.js,\
src/*.spec.jsx,\
src/*.spec.ts,\
src/*.spec.tsx,\
src/**/*.spec.js,\
src/**/*.spec.jsx,\
src/**/*.spec.ts,\
src/**/*.spec.tsx,\
output-target/*.spec.ts,\
.md,\
*.mdx,\
src/**/stories/**,\
Expand All @@ -42,4 +45,4 @@ src/typings.d.ts,\
src/index.ts,\

sonar.typescript.tsconfigPath=tsconfig.json
sonar.javascript.lcov.reportPaths=coverage/lcov.info
sonar.javascript.lcov.reportPaths=coverage/lcov.info
5 changes: 5 additions & 0 deletions packages/core/stencil.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { sass } from '@stencil/sass'

import { reactOutputTarget } from '@stencil/react-output-target'
import { vueOutputTarget } from '@stencil/vue-output-target'
import { reactBooleanFixOutputTarget } from './output-target/react-boolean'

export const config: Config = {
namespace: 'core',
Expand Down Expand Up @@ -123,6 +124,10 @@ export const config: Config = {
'ion-toolbar',
],
}),
reactBooleanFixOutputTarget({
attachPropsFile:
'../../react/src/components/react-component-lib/utils/attachProps.ts',
}),
{
type: 'dist',
esmLoaderPath: '../loader',
Expand Down

0 comments on commit 373d16e

Please sign in to comment.