Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix noFallthroughCasesInSwitch/jsx object is not extensible #9921

Merged
merged 2 commits into from
Oct 30, 2020
Merged

Fix noFallthroughCasesInSwitch/jsx object is not extensible #9921

merged 2 commits into from
Oct 30, 2020

Conversation

ryota-murakami
Copy link
Contributor

Relative Issues #9868 #9429

I fixed broken yarn start that cause react-scripts can't handle correctly noFallthroughCasesInSwitch and jsx compileOptions.

Oct-27-2020 16-12-58

Thank you for your decent review 😀

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

@iansu @ianschmitz I've had this reported to me privately too, this seems like a change to the behaviour of ts.readConfigFile where the object is no longer extensible.

@mrmckeb mrmckeb closed this Oct 27, 2020
@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 27, 2020

Re-running CI.

@mrmckeb mrmckeb reopened this Oct 27, 2020
@benneq
Copy link
Contributor

benneq commented Oct 27, 2020

@ryota-murakami Have you check the inner properties of compilerOptions? Those should be frozen, too. Though a simple appTsConfig.compilerOptions = { ...appTsConfig.compilerOptions }; might not work.

@ianschmitz ianschmitz added this to the 4.0.1 milestone Oct 28, 2020
@ryota-murakami
Copy link
Contributor Author

@benneq @KonstantinSimeonov I'm sorry for slow response.

Have you check the inner properties of compilerOptions? Those should be frozen, too. Though a simple appTsConfig.compilerOptions = { ...appTsConfig.compilerOptions }; might not work.

I've checked inner properties of compilerOptions are frozen.
Bellow screenshot showing Object.isFrozen(appTsConfig.compilerOptions.module) return true after destructured property of appTsConfig.compilerOptions on new Object.

appConfig

And I'll share video clip that recorded running bellow code without Error.

  if (appTsConfig.compilerOptions == null) {
    appTsConfig.compilerOptions = {};
    firstTimeSetup = true;
  } else {
    appTsConfig.compilerOptions = { ...appTsConfig.compilerOptions };
    for(const option in appTsConfig.compilerOptions) {
      if (!Object.isFrozen(option)) new Error('appTsConfig.compilerOptions option should be frozen')
    }
  }

https://www.dropbox.com/s/912pp3xudr9rn9s/Screen%20Recording%202020-10-30%20at%2021.40.54.mov?dl=0

@KonstantinSimeonov
Copy link
Contributor

KonstantinSimeonov commented Oct 30, 2020

@ryota-murakami

    for(const option in appTsConfig.compilerOptions) {
      if (!Object.isFrozen(option)) new Error('appTsConfig.compilerOptions option should be frozen')
    }

This is iterating over the keys of the object, which are strings and are always frozen and is not checking whether the properties are frozen or not 😛 . I think @benneq 's idea was to make sure that no crashes would occur if any of appTsConfig.compilerOptions's properties is frozen. Although I think that this is unlikely, you can safeguard against it by deep cloning the whole object.

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
@ryota-murakami
Copy link
Contributor Author

@KonstantinSimeonov Thanks, that iterator is garbage 😅
And merge your suggested solution. I confirm that working.

But I don't understand why guys care about property frozen status?
This problem root is Object isn't extensible, co script couldn't add suggested config.
I think it doesn't matter frozen status 🤔

@ianschmitz ianschmitz changed the title 4.0.0 Fix noFallthroughCasesInSwitch/jsx object is not extensible err… Fix noFallthroughCasesInSwitch/jsx object is not extensible Oct 30, 2020
@ianschmitz ianschmitz merged commit 3a98ed1 into facebook:master Oct 30, 2020
@ianschmitz
Copy link
Contributor

Thanks folks!

@ryota-murakami
Copy link
Contributor Author

ryota-murakami commented Nov 1, 2020

@benneq @KonstantinSimeonov I got it why should we handle isFroze() result of each compileOptions.
If isFroze(someOption) === true and original config from reading files value is different from CRA suggested,
the script couldn't apply change to suggested value.

Thank you for your support about this PR!

@vegawong
Copy link

@ianschmitz should it make a new release version?

oklas added a commit to oklas/react-app-alias that referenced this pull request Nov 21, 2020
@cseas
Copy link

cseas commented Nov 22, 2020

Can we please have a release with this fix? The problem is occurring for a lot of people in #9868

blackarctic added a commit to blackarctic/create-react-app that referenced this pull request Nov 25, 2020
* Fix noFallthroughCasesInSwitch/jsx object is not extensible (facebook#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>

* Add logo license to README

* Remove trailing space in reportWebVitals.ts (facebook#10040)

* docs: add React Testing Library as a library requiring jsdom (facebook#10052)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Increase Workbox's maximumFileSizeToCacheInBytes (facebook#10048)

* Create FUNDING.yml

* replace inquirer with prompts (facebook#10083)

- remove `react-dev-utils/inquirer` public import

* Prepare 4.0.1 release

* Prepare 4.0.1 release

* Publish

 - cra-template-typescript@1.1.1
 - cra-template@1.1.1
 - create-react-app@4.0.1
 - react-dev-utils@11.0.1
 - react-scripts@4.0.1

Co-authored-by: Ryota Murakami <dojce1048@gmail.com>
Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
Co-authored-by: Ian Sutherland <ian@iansutherland.ca>
Co-authored-by: sho90 <aznecosann@gmail.com>
Co-authored-by: Anyul Rivas <anyulled@gmail.com>
Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>
Co-authored-by: Jeffrey Posnick <jeffy@google.com>
Co-authored-by: Evan Bacon <baconbrix@gmail.com>
blackarctic added a commit to blackarctic/create-react-app that referenced this pull request Apr 29, 2021
* Fix noFallthroughCasesInSwitch/jsx object is not extensible (facebook#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>

* Add logo license to README

* Remove trailing space in reportWebVitals.ts (facebook#10040)

* docs: add React Testing Library as a library requiring jsdom (facebook#10052)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Increase Workbox's maximumFileSizeToCacheInBytes (facebook#10048)

* Create FUNDING.yml

* replace inquirer with prompts (facebook#10083)

- remove `react-dev-utils/inquirer` public import

* Prepare 4.0.1 release

* Prepare 4.0.1 release

* Publish

 - cra-template-typescript@1.1.1
 - cra-template@1.1.1
 - create-react-app@4.0.1
 - react-dev-utils@11.0.1
 - react-scripts@4.0.1

* chore: bump web-vital dependency version (facebook#10143)

* chore: bump typescript version (facebook#10141)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Add TypeScript 4.x as peerDependency to react-scripts(facebook#9964)

* remove chalk from formatWebpackMessages (facebook#10198)

* Upgrade @svgr/webpack to fix build error (facebook#10213)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Improve vendor chunk names in development (facebook#9569)

* Update postcss packages (facebook#10003)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Recovered some integration tests (facebook#10091)

* Upgrade sass-loader (facebook#9988)

* Move ESLint cache file into node_modules (facebook#9977)

Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>

* Revert "Update postcss packages" (facebook#10216)

This reverts commit 580ed5d.

* Remove references to Node 8 (facebook#10214)

* fix(react-scripts): add missing peer dependency react and update react-refresh-webpack-plugin (facebook#9872)

* Update using-the-public-folder.md (facebook#10314)

Some library --> Some libraries

* docs: add missing override options for Jest config (facebook#9473)

* Fix CI tests (facebook#10217)

* appTsConfig immutability handling by immer (facebook#10027)

Co-authored-by: mad-jose <joset@yeswearemad.com>

* Add support for new BUILD_PATH advanced configuration variable (facebook#8986)

* Add opt-out for eslint-webpack-plugin (facebook#10170)

* Prepare 4.0.2 release

* Publish

 - cra-template-typescript@1.1.2
 - cra-template@1.1.2
 - create-react-app@4.0.2
 - react-dev-utils@11.0.2
 - react-error-overlay@6.0.9
 - react-scripts@4.0.2

* tests: update test case to match the description (facebook#10384)

* Bump webpack-dev-server 3.11.0 -> 3.11.1 (facebook#10312)

Resolves facebook#10084 security vulnerability in websocket-driver library version 0.5.6, imported transitively by sockjs

* Upgrade eslint-webpack-plugin to fix opt-out flag (facebook#10590)

* update immer to 8.0.1 to address vulnerability (facebook#10412)

Resolves facebook#10411

Bumps immer version to 8.0.1 to address the prototype pollution
vulnerability with the current 7.0.9 version.

* Prepare 4.0.3 release

* Update CHANGELOG

* Publish

 - create-react-app@4.0.3
 - react-dev-utils@11.0.3
 - react-scripts@4.0.3

Co-authored-by: Ryota Murakami <dojce1048@gmail.com>
Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
Co-authored-by: Ian Sutherland <ian@iansutherland.ca>
Co-authored-by: sho90 <aznecosann@gmail.com>
Co-authored-by: Anyul Rivas <anyulled@gmail.com>
Co-authored-by: Ian Schmitz <ianschmitz@gmail.com>
Co-authored-by: Jeffrey Posnick <jeffy@google.com>
Co-authored-by: Evan Bacon <baconbrix@gmail.com>
Co-authored-by: Sahil Purav <sahil5684@gmail.com>
Co-authored-by: Hakjoon Sim <trainto@gmail.com>
Co-authored-by: Chris Shepherd <chris@chrisshepherd.me>
Co-authored-by: Jason Williams <936006+jasonwilliams@users.noreply.github.com>
Co-authored-by: Jabran Rafique⚡️ <jabranr@users.noreply.github.com>
Co-authored-by: John Ruble <johnruble@gmail.com>
Co-authored-by: Morten N.O. Nørgaard Henriksen <morten.n.o.henriksen@icloud.com>
Co-authored-by: Sergey Makarov <serega.s.makar@gmail.com>
Co-authored-by: EhsanKhaki <ehsankhfr@gmail.com>
Co-authored-by: Kristoffer K <merceyz@users.noreply.github.com>
Co-authored-by: Aviv Hadar <Avivhdr@gmail.com>
Co-authored-by: Tobias Büschel <13087421+tobiasbueschel@users.noreply.github.com>
Co-authored-by: mad-jose <44253495+josezone@users.noreply.github.com>
Co-authored-by: mad-jose <joset@yeswearemad.com>
Co-authored-by: Andrew Hyndman <ajhyndman@hotmail.com>
Co-authored-by: Brody McKee <mrmckeb@users.noreply.github.com>
Co-authored-by: James George <jamesgeorge998001@gmail.com>
Co-authored-by: Dion Woolley <woolley.dion@gmail.com>
Co-authored-by: Walker Clem <51654951+wclem4@users.noreply.github.com>
wombleton pushed a commit to AurorNZ/create-react-app that referenced this pull request Jun 1, 2021
…#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
abhiisheek pushed a commit to abhiisheek/create-react-app that referenced this pull request May 19, 2023
…#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
abhiisheek pushed a commit to abhiisheek/create-react-app that referenced this pull request May 24, 2023
…#9921)

Co-authored-by: Konstantin Simeonov <kon.simeonov@protonmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants