Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Changes fetchye into a monorepo #15

Merged
merged 51 commits into from
Oct 30, 2020
Merged

Changes fetchye into a monorepo #15

merged 51 commits into from
Oct 30, 2020

Conversation

JamesSingleton
Copy link
Contributor

@JamesSingleton JamesSingleton commented Oct 7, 2020

I am opening this PR as a draft because I need to still update the documentation and workflows. However, I would like everyone to try the package(s) out.

@JamesSingleton JamesSingleton requested a review from a team as a code owner October 7, 2020 17:41
@JamesSingleton JamesSingleton marked this pull request as draft October 7, 2020 17:41
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 8, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2b7f045:

Sandbox Source
quick-install Configuration
fetchye-provider-install Configuration
fetchye-redux-provider-install Configuration
nextjs-fetchye-ssr Configuration

packages/fetchye-immutable-cache/package.json Outdated Show resolved Hide resolved
lerna.json Show resolved Hide resolved
packages/fetchye-immutable-cache/rollup.config.js Outdated Show resolved Hide resolved
packages/fetchye/package.json Outdated Show resolved Hide resolved
packages/fetchye/rollup.config.js Outdated Show resolved Hide resolved
packages/fetchye/rollup.config.js Outdated Show resolved Hide resolved
@JAdshead
Copy link
Contributor

JAdshead commented Oct 8, 2020

FetchyeReduxProvider needs also be its own package

@JamesSingleton
Copy link
Contributor Author

FetchyeReduxProvider needs also be its own package

Ok perfect, I wasn't entirely sure about that.

packages/fetchye-immutable-cache/package.json Show resolved Hide resolved
packages/fetchye/package.json Show resolved Hide resolved
packages/fetchye/package.json Outdated Show resolved Hide resolved
.codesandbox/ci.json Outdated Show resolved Hide resolved
@JamesSingleton JamesSingleton marked this pull request as ready for review October 19, 2020 17:13
@JamesSingleton JamesSingleton changed the title WIP: Changes fetchye into a monorepo and adds fetchye-immutable-cache Changes fetchye into a monorepo and adds fetchye-immutable-cache Oct 19, 2020
@merceyz
Copy link

merceyz commented Oct 19, 2020

Hey @JamesSingleton 👋
Saw your tweet, what specifically is the issue with Yarn?

I cloned the repo, checked out your branch, set it to use yarn 2, added some missing dependencies, and ran the build, worked perfectly fine.

diff --git a/packages/fetchye-immutable-cache/package.json b/packages/fetchye-immutable-cache/package.json
index 160884a..f260e67 100644
--- a/packages/fetchye-immutable-cache/package.json
+++ b/packages/fetchye-immutable-cache/package.json
@@ -36,9 +36,11 @@
     "immutable": "^4.0.0-rc.12"
   },
   "devDependencies": {
+    "@babel/core": "7.11.6",
     "@rollup/plugin-babel": "^5.2.1",
     "@rollup/plugin-commonjs": "^15.1.0",
     "@rollup/plugin-node-resolve": "^9.0.0",
+    "babel-preset-amex": "^3.4.1",
     "fetchye": "*",
     "rimraf": "^3.0.2",
     "rollup": "^2.28.2",
diff --git a/packages/fetchye-redux-provider/package.json b/packages/fetchye-redux-provider/package.json
index 0683e5e..576bf25 100644
--- a/packages/fetchye-redux-provider/package.json
+++ b/packages/fetchye-redux-provider/package.json
@@ -39,10 +39,12 @@
     "redux": "^4.0.5"
   },
   "devDependencies": {
+    "@babel/core": "7.11.6",
     "@rollup/plugin-babel": "^5.2.1",
     "@rollup/plugin-commonjs": "^15.1.0",
     "@rollup/plugin-node-resolve": "^9.0.0",
     "@testing-library/react": "^11.0.4",
+    "babel-preset-amex": "^3.4.1",
     "fetchye": "*",
     "rimraf": "^3.0.2",
     "rollup": "^2.29.0",
diff --git a/packages/fetchye/package.json b/packages/fetchye/package.json
index 8a4205f..17ddda7 100644
--- a/packages/fetchye/package.json
+++ b/packages/fetchye/package.json
@@ -59,11 +59,13 @@
     "react-dom": "^16.13.1"
   },
   "devDependencies": {
+    "@babel/core": "7.11.6",
     "@babel/plugin-transform-runtime": "^7.11.5",
     "@rollup/plugin-babel": "^5.2.1",
     "@rollup/plugin-commonjs": "^15.1.0",
     "@rollup/plugin-node-resolve": "^9.0.0",
     "@testing-library/react": "^11.0.4",
+    "babel-preset-amex": "^3.4.1",
     "react": "^16.13.1",
     "react-dom": "^16.13.1",
     "react-redux": "^7.2.1",

@JamesSingleton
Copy link
Contributor Author

@merceyz this is using Yarn 1. The issue currently is having to disable import/no-unresolved for fetchye-redux-provider and fetchye-immutable-cache

@merceyz
Copy link

merceyz commented Oct 19, 2020

I'm aware, but it's way easier to test with V2 as its node_modules linker is more stable, but I digress.

The issue you're seeing is with the eslint plugin, not yarn.
import-js/eslint-plugin-import#1174
import-js/eslint-plugin-import#1650

@JamesSingleton
Copy link
Contributor Author

@merceyz good to know! For some reason I thought it was Yarn not resolving. Also, any particular reason for adding @babel/core directly even though babel-preset-amex brings in @babel/core?

@merceyz merceyz mentioned this pull request Oct 19, 2020
@merceyz
Copy link

merceyz commented Oct 19, 2020

Also, any particular reason for adding @babel/core directly even though babel-preset-amex brings in @babel/core?

Yes, @rollup/plugin-babel has a peer dependency on @babel/core and it is not satisfied by babel-preset-amex. It Just Works™ because of hoisting but is really fragile, read https://yarnpkg.com/advanced/rulebook#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies for a more in depth explanation.

I opened #16 (which targets this branch) to "show" the use of V2, you can close it if you want, but I highly suggest using it as its node_modules linker is more stable than V1, and makes it just a flip of a switch to adopt PnP in the future (https://yarnpkg.com/features/pnp).

You can run with PnP on the CI to catch dependency issues and node_modules locally to preserve the DX

@merceyz
Copy link

merceyz commented Oct 19, 2020

Don't think lockfile-lint is needed, the lockfile doesn't store the URL and it will only fetch from the registry specified in .yarnrc.yml

@JamesSingleton
Copy link
Contributor Author

Don't think lockfile-lint is needed, the lockfile doesn't store the URL and it will only fetch from the registry specified in .yarnrc.yml

@merceyz so does that mean dependencies with Yarn 2 can't be hijacked in the same manner as Yarn 1 or a package-lock.json?

@merceyz
Copy link

merceyz commented Oct 19, 2020

It might be possible, but it's harder, for example all http traffic is blocked by default.

https://yarnpkg.com/configuration/yarnrc#unsafeHttpWhitelist
yarnpkg/berry#416

package.json Show resolved Hide resolved
@merceyz
Copy link

merceyz commented Oct 22, 2020

Don't think lockfile-lint is needed, the lockfile doesn't store the URL and it will only fetch from the registry specified in .yarnrc.yml

@merceyz so does that mean dependencies with Yarn 2 can't be hijacked in the same manner as Yarn 1 or a package-lock.json?

Yarn will support blocking network requests on a per hostname basis with yarnpkg/berry#2030, that should hopefully cover the "hijack" concerns

README.md Outdated Show resolved Hide resolved
"license": "Apache-2.0",
"main": "es/index.js",
"files": [
"es"
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean it can technically be anything

const inputSrc = 'src/index.js';

export default [
{
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably only need a CJS build for this as its a test lib

Copy link
Contributor

@JAdshead JAdshead left a comment

Choose a reason for hiding this comment

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

we will need to fast followup with fixes to the builds

@JamesSingleton JamesSingleton merged commit ba6121c into beta Oct 30, 2020
@JamesSingleton JamesSingleton deleted the chore/rollup-config branch October 30, 2020 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants