add dependency on core-js@^3.6.5 #359
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #359 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 208 208
Branches 42 42
=========================================
Hits 208 208 Continue to review full report at Codecov.
|
chrisbobbe
added a commit
to zulip/zulip-mobile
that referenced
this pull request
Aug 11, 2020
This reverts commit 01593b3. We should redo the upgrade when we can take a version of `prettier-eslint` in which `core-js` is re-introduced as a dependency; prettier/prettier-eslint#348 is the closest issue for that. The commands listed in 01593b3 should make it easy to redo the upgrade mechanically, and without having to think about rebase conflicts. The version (11.0.0) of `prettier-eslint` that we got in 01593b3 was affected by a bug with symptoms quite similar to ones we see with the Prettier VSCode extension (a.k.a. `esbenp.prettier-vscode`) at version 5 and above [1]. We'll have to resolve the problem with the VSCode extension at some point (see discussion for a likely way we'll do that), since we can't assume it will always be fine to stay below version 5, and version 5 causes problems because of an intentional design change. But, from experimentation, it appears that this time the problem is just caused by `prettier-eslint` not having the right version of `core-js`. At 10.1.0, the latest version before the problem starts happening, we can remove `node_modules/prettier-eslint/node_modules/core-js`, and we see the problem. Version 10.1.1, which ships with the problem, removed `core-js` as a dependency, and an issue with a different bad symptom (prettier/prettier-eslint#348) was soon filed, and a PR (prettier/prettier-eslint#359) came along to add `core-js` back in. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/SOLVED.3A.20Prettier.20auto-formatting/near/893164
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Aug 11, 2020
This reverts commit 01593b3. We should redo the upgrade when we can take a version of `prettier-eslint` in which `core-js` is re-introduced as a dependency; prettier/prettier-eslint#348 is the closest issue for that. The commands listed in 01593b3 should make it easy to redo the upgrade mechanically, and without having to think about rebase conflicts. The version (11.0.0) of `prettier-eslint` that we got in 01593b3 was affected by a bug with symptoms quite similar to ones we see with the Prettier VSCode extension (a.k.a. `esbenp.prettier-vscode`) at version 5 and above [1]. We'll have to resolve the problem with the VSCode extension at some point (see discussion for a likely way we'll do that), since we can't assume it will always be fine to stay below version 5, and version 5 causes problems because of an intentional design change. But, from experimentation, it appears that this time the problem is just caused by `prettier-eslint` not having the right version of `core-js`. At 10.1.0, the latest version before the problem starts happening, we can remove `node_modules/prettier-eslint/node_modules/core-js`, and we see the problem. Version 10.1.1, which ships with the problem, removed `core-js` as a dependency, and an issue with a different bad symptom (prettier/prettier-eslint#348) was soon filed, and a PR (prettier/prettier-eslint#359) came along to add `core-js` back in. We can't just back up to version 10.1.0, because then we're at risk of it not working with ESLint 6 [2]. So, just revert the entire ESLint 6 upgrade. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/SOLVED.3A.20Prettier.20auto-formatting/near/893164 [2] prettier/prettier-eslint#338 (comment)
gnprice
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Aug 11, 2020
This reverts commit 01593b3. We should redo the upgrade when we can take a version of `prettier-eslint` in which `core-js` is re-introduced as a dependency; prettier/prettier-eslint#348 is the closest issue for that. The commands listed in 01593b3 should make it easy to redo the upgrade mechanically, and without having to think about rebase conflicts. The version (11.0.0) of `prettier-eslint` that we got in 01593b3 was affected by a bug with symptoms quite similar to ones we see with the Prettier VSCode extension (a.k.a. `esbenp.prettier-vscode`) at version 5 and above [1]. We'll have to resolve the problem with the VSCode extension at some point (see discussion for a likely way we'll do that), since we can't assume it will always be fine to stay below version 5, and version 5 causes problems because of an intentional design change. But, from experimentation, it appears that this time the problem is just caused by `prettier-eslint` not having the right version of `core-js`. At 10.1.0, the latest version before the problem starts happening, we can remove `node_modules/prettier-eslint/node_modules/core-js`, and we see the problem. Version 10.1.1, which ships with the problem, removed `core-js` as a dependency, and an issue with a different bad symptom (prettier/prettier-eslint#348) was soon filed, and a PR (prettier/prettier-eslint#359) came along to add `core-js` back in. We can't just back up to version 10.1.0, because then we're at risk of it not working with ESLint 6 [2]. So, just revert the entire ESLint 6 upgrade. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/SOLVED.3A.20Prettier.20auto-formatting/near/893164 [2] prettier/prettier-eslint#338 (comment)
chrisbobbe
added a commit
to zulip/zulip-mobile
that referenced
this pull request
Aug 12, 2020
A redo of 01593b3, after we reverted it in e3e8d9a. One thing we realized [1] is that we don't have to wait for `prettier-eslint` to fix the problem of its missing `core-js` dependency; we can provide it ourselves. We do so by just adding it directly to our own `dependencies`. So, in addition to the commands listed in 01593b3, also run `yarn add core-js@^3.1.4`. We choose ^3.1.4 because that's what was there before prettier/prettier-eslint#338. The open PR I mention in e3e8d9a (prettier/prettier-eslint#359) suggests ^3.6.5, but it seems to do so at random; might as well approach as closely as possible a configuration that we know works. Now, we have the ESLint upgrade, and format-on-save works as expected. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/SOLVED.3A.20Prettier.20auto-formatting/near/982426 Fixes: #4120
|
Stale pull request |
|
@zimme @kentcdodds Do you mind merging and publishing a minor version? Multiple editor formatters would need to install core-js separately to use |
|
I haven't worked on this project in years. I'm definitely not fit to merge any pull requests at this point. Sorry. 😬 If there are no other maintainers who can merge pull requests, then let me know and I'll give somebody the rights. |
|
Seems @hamzahamidi is still doing maintenance. |
|
Hello, would be nice to have this one merged! @hamzahamidi @kentcdodds any way we can help? |
|
I'm not fan of adding core-js as a dependency, maybe as a peer-dependency instead? |
Codecov Report
@@ Coverage Diff @@
## master #359 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 208 208
Branches 42 42
=========================================
Hits 208 208 Continue to review full report at Codecov.
|
|
I think best way is to compile without corejs (as this is node library) or target newer version of node.js (as required polyfills no longer needed). All features should be available in node 10 |
I think Node 10 is now targeted: 8f35b25 (I haven't independently verified that this solves anything, but it sound like it might?) |
|
Oh I think this can be closed because core-js stopped being imported in #539. Is that right? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using
@babel/preset-envyou need to take a dependency oncore-jsbecause it will inject runtime references in eitherentryorusage(in this case we are usingusage).https://babeljs.io/docs/en/babel-preset-env#usebuiltins
Fixes #348