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

upgrade eslint configuration to 8.x #4178

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

haroun
Copy link
Contributor

@haroun haroun commented May 29, 2023

Summary

Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"

What are the specific steps to test this change?

For example:

  1. Run the website and log in as an admin
  2. Open a piece manager modal and select several pieces
  3. Click the "Archive" button on the top left of the manager and confirm that it should proceed
  4. Check that all pieces have been archived properly

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

@haroun haroun self-assigned this May 29, 2023
@linear
Copy link

linear bot commented May 29, 2023

PRO-4227 Find a solution for the issues encountered when trying to use pnpm in a mono-repo approach with Apostrophe.

Michelin reported that they are encountering an issue when trying to use pnpm in a mono-repo approach with Apostrophe.

Here is the GitHub-related issue, which includes complete Steps to Reproduce: #4126

They need to use pnpm for our their "next-gen" modules and applications, and so we need to fix this somewhat urgently ("somewhat urgently" means in the next couple of weeks).

The problem explained

The major difference (from the perspective of Apostrophe app) between npm and pnpm installation is the way node modules are stored, and in a result of that how they are resolved later by the NodeJS core resolver.

The very simplified explanation (and not 100% accurate, but close to the reality):

  • npm- modules are resolved downwards (the not 100% accurate part)
  • pnpm - modules are resolved upwards

Another interesting discovery is that npm can install similar to pnpmpackage structure (explained below):

npm install –global-style

When npm installs packages, the dependencies of the direct dependencies are "copied" to the application node_modules folder. For example, lodash module can be found in application's node_modules/lodashalthough no direct app dependency is introduced. The lodash package can be found in one more location - node_modules/apostrophe/lodashwhich is the original dependency.

The application can require('lodash')and it will be OK UNTIL Apostrophe removes that dependency. When this happens the application will crash.

In this scenario resolving happens downwards - the indirect dependencies are found in application root's node_modules/

When pnpm installs packages, the application node_modules/folder contains ONLY the direct dependencies. From the above example, node_modules/lodash is not to be found. Additionally, node_modules/apostrophe/node_modules/lodash is also not there (in fact apostrophe/node_modules folder contains no packages). So how it works?.

  • node_modules/apostrophe is a symlink to a folder similar to node_modules/.pnpm/apostrophe3.47.0_tiptap+core2.0.3_tiptap+pm2.0.3_postcss8.4.23/node_modules/apostrophe
  • the contents of the LAST `node_modules` above are all packages that are direct dependency of Apostrophe, including apostrophe itself. This very strange path represents Apostrophe version and its peer dependencies unique identifier.
  • module resolution happens upwards - the Aapostrophe owned node_modules is empty, so the upper node_modules is used and indeed required packages are there

as a result of this "trick" Apostrophe is actually able to resolve application level packages (not that it's a possible scenario, but this is the case) but it can't rely on hoisted indirect dependencies downwards (from its own dependencies). Kinda "dependency sandbox".

Specific Apostrophe problems related with how pnpm does it

As a start, pnpm caught instantly bad dependencies, introduced with the eslint config module:

Screenshot from 2023-05-22 16-10-34.png

Beside this Apostrophe should be generally in a good place. The problem would be if the core or any core plugin relies on the described above npm package hoisting - imports a module that is not a direct package.json dependency (lodash is a good candidate btw). On a first glance, the specific apostrophe module resolution should work as expected as well.

The troubles seems to be only build related.

The "magic solution" compiler.options.context

The first thing that usually fails (on build) is dependencies of vue-loaderplugin. After some debugging, the reason behind it is the way vue-loader is resolving (almost everything). It uses compiler.options.context (basically the context webpack configuration value, which defaults to pwd) as the path argument of require.resolve(). The only way to overcome that is to introduce a explicit context config value that points to the actual apostrophe package folder, but the price in doing it is high!

The PoC

Using a standard a3-boilerplate clone I was able to fix some build issues with "monkey patching" the core apos module assset/lib/webpack/apos/webpack.config.js (and the similar one for src) with this Webpack config line:

context: path.resolve(__dirname, '../../../../../'), // the current Apostrophe folder

The context options is used for resolving stuff, the base path of doing that. vue-loader (probably other modules too) relies solely on this configuration and ignores everything else (like resolveLoader, resolve configurations). This is the only way I was able to let apostrophe (almost) build, or at least not fail early. In the time of writing this, I'm fighting the other resolvers introduced in our webpack configs. The problem here is that application level imports in the root apos-build folder are broken (tiptap is failing first). This should be solvable with adapting the resolve statements in the webpack configs, more on it later.

The real problem is the context. Changing it can do harm and needs to be tested and assessed. It basically changes the building context from application root to the apost package folder - for sure not a BC change.

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

1 participant