Fix monorepo support by allowing import to do what it's good at (and … #2359

NullVoxPopuli · 2022-01-28T15:54:18Z

…ignoring paths 🙃)

NullVoxPopuli · 2022-01-28T15:56:15Z

lib/get-config.js

+  let plugin;
+
+  try {
+    let importedModule = await import(pluginName);


Without trying this first, we can't actually use plugins from node_modules 🙈

For example, prior to this change, we'd get this error:

<repo>/node_modules/resolve/lib/sync.js:102 var err = new Error("Cannot find module '" + x + "' from '" + parent + "'"); ^ Error: Cannot find module 'ember-template-lint-plugin-tailwindcss' from '<repo>/frontend' at Function.resolveSync [as sync] (<repo>/node_modules/resolve/lib/sync.js:102:15) at requirePlugin (file://<repo>/node_modules/ember-template-lint/lib/get-config.js:31:28) at processPlugins (file://<repo>/node_modules/ember-template-lint/lib/get-config.js:150:22) at getProjectConfig (file://<repo>/node_modules/ember-template-lint/lib/get-config.js:396:28) at async Linter.loadConfig (file://<repo>/node_modules/ember-template-lint/lib/linter.js:48:19) at async Linter.verify (file://<repo>/node_modules/ember-template-lint/lib/linter.js:304:7) at async run (file://<repo>/node_modules/ember-template-lint/bin/ember-template-lint.js:211:21) { code: 'MODULE_NOT_FOUND' } error Command failed with exit code 1. }

Can you explain more about how the monorepo is setup?

The issue is present in any monorepo where the ember project is not at the root, where the root's node_modules contains ember-template-lint (so almost every monorepo)

the reproduction is very easy:

clone any monorepo

cd into the monorepo

cd into the ember project

swap out template lint for v4

add a template-lint plugin that is compat with v4

error

Are you suggesting that your monorepo has ember-template-lint in the root (presumably as a devDep), and the plugins in subdirs?

I wouldn't say typically. If you have a small enough monorepo where you can have the same version of a tool everywhere and every package in the monorepo needed it, it may make sense.
But monorepo conventions are orthogonal to the issue here 🙃

It's perfectly reasonable for only one package in a monorepo to need template lint, and therefor, it'd be local to that package, yet potentially still hoisted

OK sure. I suppose from my perspective that's part of the benefit of monorepos - minimizing tool dependencies so you have less maintenance. But of course that's not super relevant, though what I was getting at was trying to first determine if this was an issue that could be handled via a recommended setup vs. a code change.

makes sense -- important question though -- why were we not relying on the built-in node resolution algo (which happens for us via import/require) and trying to find deps ourselves?

why were we not relying on the built-in node resolution algo (which happens for us via import/require) and trying to find deps ourselves?

Because the node resolution algorithm is actually wrong in this case 😸. We don't want to resolve the plugin name relative to ember-template-lint. Specifically, the node modules algorithm would suggest that the plugin must be resolvable from whatever location ember-template-lint itself is installed at. Which is not correct! The plugin will be installed somewhere relative to the .template-lintrc.js file not where ever** ember-template-lint happens to be installed. This is the point of the current resolve.sync code.

I disagree. And in the mean time, all plugin usage is broken in monorepos 😢

bmish · 2022-01-29T17:46:24Z

We'll need tests for this that fail before the change and pass after.

NullVoxPopuli · 2022-01-29T17:53:06Z

We'll need tests for this that fail before the change and pass after.

How do you want that to happen?
Are there existing tests where you generate whole projects and additional plugins?

bmish · 2022-01-29T18:07:26Z

@NullVoxPopuli yes we have tests / test fixtures that set up fake projects and plugins. We will need to do something similar for this monorepo situation.

NullVoxPopuli · 2022-01-29T20:27:47Z

@bmish I've added the beginnings of a test, the main thing is that I need to set the baseDir of a Project and that's not supported in the current version of fixurify project -- and "just upgrading" breaks a bunch of stuff.

NullVoxPopuli · 2022-01-29T20:37:16Z

@bmish anyway to move forward without the test? or is there some other way to simulate monorepo? I suppose I could not use fixturify-project for the monorepo test 🤔

This is just blocking adoption within all monorepos 🙃

imo, letting node do the module resolution by default (instead of via file path) is the more correct thing to do anyway 🥳

bmish · 2022-01-30T16:50:11Z

Having tests for all changes is very important to me, especially for a change like this. I made sure all the changes that went into the v4 release were comprehensively tested. So it will be great for us to get this monorepo scenario tested. I can try to help with the fixturify-project upgrade over the next few days if you haven't figured that out. Thanks for starting on the monorepo test in the meantime.

NullVoxPopuli · 2022-01-30T16:57:11Z

Having tests for all changes is very important to me

I'm with ya there -- especially with big projects.

I don't think I want to deal with fixturify changes 🙃
Should I wait for you to upgrade that? or should I proceed with these tests without fixturify?

bmish · 2022-01-31T15:48:53Z

My preferred path is that we get the fixturify-project upgrade in so we can utilize that. However, I spent a bit of time trying to figure it out but didn't make much progress and it was not fun...I wish there was a better migration guide for it. I'll see if I can try again in next few days.

bmish · 2022-03-04T16:16:10Z

Tests for this should hopefully be unblocked now that this has been merged:

Update fixturify-project to v4 #2407

…ignoring paths 🙃)

…epos

rwjblue · 2022-04-14T17:42:44Z

Taking another crack at this over in #2471

rwjblue · 2022-04-14T22:08:33Z

More tests being added in #2473

rwjblue · 2022-04-14T22:22:45Z

At the moment, I'm not able to reproduce a failure (in either #2471 or #2473). I think we need a better understanding of the failure (preferably via a reproduction in the "clone this, npm install, npm test" form) that @NullVoxPopuli was trying to fix.

NullVoxPopuli · 2022-04-14T22:45:40Z

full repro steps:

Terminal 1

git clone git@github.com:NullVoxPopuli/limber.git
cd limber
git checkout etl-repro
yarn

This branch, etl-repro has a template-lint plugin and ember-template-lint@^4.4.0.

Below is where we get the ESM version of the plugin, as plugins gotta be ESM

Terminal 2

git@gitlab.com:NullVoxPopuli/ember-template-lint-plugin-tailwindcss.git
cd ember-template-lint-plugin-tailwindcss
git checkout vitest
yarn
yarn build
yarn link

Back in Terminal 1

yarn link ember-template-lint-plugin-tailwindcss
cd frontend
yarn link ember-template-lint-plugin-tailwindcss
yarn lint:hbs

Expected Error:

❯ yarn ember-template-lint .
yarn run v1.22.17
$ <repo>/node_modules/.bin/ember-template-lint .
<repo>/node_modules/resolve/lib/sync.js:111
    var err = new Error("Cannot find module '" + x + "' from '" + parent + "'");
              ^

Error: Cannot find module 'ember-template-lint-plugin-tailwindcss' from '<repo>/frontend'
    at Function.resolveSync [as sync] (<repo>/node_modules/resolve/lib/sync.js:111:15)
    at requirePlugin (file://<repo>/node_modules/ember-template-lint/lib/get-config.js:39:28)
    at processPlugins (file://<repo>/node_modules/ember-template-lint/lib/get-config.js:159:22)
    at getProjectConfig (file://<repo>/node_modules/ember-template-lint/lib/get-config.js:419:28)
    at async Linter.loadConfig (file://<repo>/node_modules/ember-template-lint/lib/linter.js:45:19)
    at async Linter.verify (file://<repo>/node_modules/ember-template-lint/lib/linter.js:301:7)
    at async run (file://<repo>/node_modules/ember-template-lint/bin/ember-template-lint.js:219:21) {
  code: 'MODULE_NOT_FOUND'
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.}

rwjblue · 2022-04-15T20:18:14Z

Thank you @NullVoxPopuli! Finally reproduced this style of failure over in #2475.

The good news is that it is specifically limited to cases where resolve.sync does not find a main entry point, but import() would. That is mostly going to be when you have exports setup but no main (but not only that scenario, that just seems like the most likely one).

NullVoxPopuli · 2022-05-19T00:28:53Z

The good news is that it is specifically limited to cases where resolve.sync does not find a main entry point, but import() would. That is mostly going to be when you have exports setup but no main (but not only that scenario, that just seems like the most likely one).

I missed this.
Hmm.

This doesn't seem as bad as i thought
Will need to take a look

NullVoxPopuli commented Jan 28, 2022

View reviewed changes

bmish added the bug label Jan 29, 2022

NullVoxPopuli mentioned this pull request Jan 29, 2022

Update fixturify-project to v4 #2360

Closed

bmish mentioned this pull request Mar 4, 2022

Update fixturify-project to v4 #2407

Merged

bmish added this to the 4.3.0 milestone Mar 4, 2022

NullVoxPopuli added 2 commits March 7, 2022 18:00

Fix monorepo support by allowing import to do what it's good at (and …

db6dead

…ignoring paths 🙃)

wip: fixturify project needs to be upgraded in order to test in monor…

9df7849

…epos

NullVoxPopuli force-pushed the fix-monorepo-support branch from a519171 to 9df7849 Compare March 7, 2022 23:02

NullVoxPopuli mentioned this pull request Apr 14, 2022

Standardize CLI tests with bin tester package #2437

Merged

rwjblue mentioned this pull request Apr 14, 2022

Add tests for plugin resolution within a monorepo setup #2471

Merged

rwjblue mentioned this pull request Apr 15, 2022

Add test for plugin using exports in package.json. #2475

Draft

NullVoxPopuli mentioned this pull request Jun 11, 2022

Add tests for monorepos with shared configuration in monorepo root #2473

Merged

NullVoxPopuli closed this Jun 11, 2022

NullVoxPopuli deleted the fix-monorepo-support branch June 11, 2022 03:10

NullVoxPopuli mentioned this pull request May 4, 2023

ember-template-lint doesn't support plugins exposed using an exports field in package.json #2878

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix monorepo support by allowing import to do what it's good at (and … #2359

Fix monorepo support by allowing import to do what it's good at (and … #2359

NullVoxPopuli commented Jan 28, 2022

NullVoxPopuli Jan 28, 2022

bmish Jan 29, 2022

NullVoxPopuli Jan 29, 2022

NullVoxPopuli Jan 29, 2022

scalvert Mar 7, 2022

NullVoxPopuli Mar 7, 2022

scalvert Mar 7, 2022

NullVoxPopuli Mar 7, 2022 •

edited

rwjblue Apr 14, 2022 •

edited

NullVoxPopuli May 19, 2022

bmish commented Jan 29, 2022

NullVoxPopuli commented Jan 29, 2022

bmish commented Jan 29, 2022

NullVoxPopuli commented Jan 29, 2022

NullVoxPopuli commented Jan 29, 2022

bmish commented Jan 30, 2022

NullVoxPopuli commented Jan 30, 2022

bmish commented Jan 31, 2022 •

edited

bmish commented Mar 4, 2022

rwjblue commented Apr 14, 2022

rwjblue commented Apr 14, 2022

rwjblue commented Apr 14, 2022

NullVoxPopuli commented Apr 14, 2022 •

edited

rwjblue commented Apr 15, 2022

NullVoxPopuli commented May 19, 2022 •

edited

Fix monorepo support by allowing import to do what it's good at (and … #2359

Fix monorepo support by allowing import to do what it's good at (and … #2359

Conversation

NullVoxPopuli commented Jan 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Mar 7, 2022 • edited

Choose a reason for hiding this comment

rwjblue Apr 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmish commented Jan 29, 2022

NullVoxPopuli commented Jan 29, 2022

bmish commented Jan 29, 2022

NullVoxPopuli commented Jan 29, 2022

NullVoxPopuli commented Jan 29, 2022

bmish commented Jan 30, 2022

NullVoxPopuli commented Jan 30, 2022

bmish commented Jan 31, 2022 • edited

bmish commented Mar 4, 2022

rwjblue commented Apr 14, 2022

rwjblue commented Apr 14, 2022

rwjblue commented Apr 14, 2022

NullVoxPopuli commented Apr 14, 2022 • edited

rwjblue commented Apr 15, 2022

NullVoxPopuli commented May 19, 2022 • edited

NullVoxPopuli Mar 7, 2022 •

edited

rwjblue Apr 14, 2022 •

edited

bmish commented Jan 31, 2022 •

edited

NullVoxPopuli commented Apr 14, 2022 •

edited

NullVoxPopuli commented May 19, 2022 •

edited