-
Notifications
You must be signed in to change notification settings - Fork 213
Conversation
I can see in the build logs that the job published packages to NPM. But why? It even didn't pass the code formatting check. And this is only Pull Request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thank you for the PR.
I much prefer Yarn over NPM and have wanted PnP support for a while.
That said:
- My understanding is that webpack 5 supports Yarn PnP out of the box, so much of this may be redundant in Neutrino 10+
- Jest needs updating to a minor newer version (but bumping the peer dep is a breaking change), so we'd have to have a docs element to this anyway
- This implementation adds the
pnp-webpack-plugin
dependency and config complexity to NPM users not just yarn
As such, I'm slightly reluctant to add a new middleware for Neutrino 9 only, which would be removed in Neutrino 10 - and think it might make sense for this to be a new section in the docs for Neutrino 9, that contains a snippet for people to add to their .neutrino.js
along with some suggestions as to Jest upgrades etc.
Then in Neutrino 10 with webpack 5, this could just be supported natively with little implementation needed on Neutrino's side.
Thoughts?
It's not a real NPM server - see: |
Here are my thoughts. Lets discuss your points
|
packages/eslint/alias-plugins.js
Outdated
if (pnpApi) { | ||
resolvedPluginPath = pnpApi.resolveRequest(pluginName, relativeFilename); | ||
} else { | ||
resolvedPluginPath = require.resolve(pluginName, { | ||
paths: [relativeFilename], | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just use require.resolve
you almost never need to use the pnpapi directly, this is one of those cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. But unfortunately __filename
gives a virtual File System path in Yarn 2 which can't work correctly in paths
options of require.resolve
. Module will be not found with an exception. Struggled with this 2 days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a reproduction for that? We have tests making sure it works. I could see that happening if you have enableGlobalCache: true
but that will be fixed in yarnpkg/berry#2068
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any place of your code in Yarn 2 environment try this console.log(require.resolve('any-of-your-dependency', { paths: [__filename] }))
. It will not find your module even if it is relative from the current file. The same code works without any problems in regular NPM environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will and it does. As I said we have tests that cover this, but just for fun I did try exactly what you said and it works, if you have a reproduction I'd love to take a look at it
Some tests that check this:
https://github.com/yarnpkg/berry/blob/f8344493b5cf3948e71211697b50a268b756d8fc/packages/acceptance-tests/pkg-tests-specs/sources/pnp.test.js#L1714
https://github.com/yarnpkg/berry/blob/f8344493b5cf3948e71211697b50a268b756d8fc/packages/acceptance-tests/pkg-tests-specs/sources/pnp.test.js#L609-L612
https://github.com/yarnpkg/berry/blob/f8344493b5cf3948e71211697b50a268b756d8fc/packages/acceptance-tests/pkg-tests-specs/sources/pnp.test.js#L647-L651
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that the mentioned test case works in the Yarn 2 project itself. But in the linked module it fails with not found dependency. As you understand I use linked packages for local environments. Such modules are defined as
{
"devDependencies": {
"@neutrinojs/eslint": "portal:C:/Projects/Projects_GitHub/neutrino-dev-fork/packages/eslint",
}
}
yarn install
is called to setup all links in .yarn/cache
and .pnp.js
And after that I can run local script "lint": "eslint ./src --ext .js,.jsx",
to run the code and test the case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes more sense. I'm fairly certain the issue you're running into was fixed in yarnpkg/berry#2068, could you try with yarn set version from sources
or by downloading the bundle
here https://github.com/yarnpkg/berry/actions/runs/346381133?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before: Yarn version v2.2.2
After: Yarn version 2.3.3-git.20201112.f5ca1c57
Result is the same in the linked package: works when require.resolve('babel-eslint')
and fails when require.resolve('babel-eslint', { paths: [__filename] })
Oops! Something went wrong! :(
ESLint: 7.9.0
Error: Cannot read config file: C:\Users\const\Desktop\yarn-test\.eslintrc.js
Error: Cannot find module 'babel-eslint'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you try using __dirname
instead?
Again, if you could provide a reproduction I'll be able to look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update:
After removing .yarn/cache
and reinstalling dependencies require.resolve('babel-eslint', { paths: [__filename] })
started to work. It was the version 2.3.3-git.20201112.f5ca1c57
.
Than I upgraded Yarn to the latest 2.4.0
and reinstalled dependencies again. And it still works. So I removed usage of PnP API as you asked
Please, look at the new section in the documentation https://github.com/constgen/neutrino-dev/tree/pnp/packages/eslint#utility-functions |
I think everything related to Yarn 2 is resolved. |
@edmorley in Webpack 5 only |
@edmorley @eliperelman can you look at final version of this PR? |
@constgen Thank you for working on this :-) Before it can be merged (and so before spending time on review), we need to have CI passing, which is blocked on Travis CI credits running out, due to Travis CI's new policy (that is, there are no more free credits). The best way to resolve this would be to migrate to another provider -- I would recommend Circle CI. I don't have time to work on such a migration, but would be happy to review a PR that did so. |
Is GitHub Actions is an option? I tried it recently for one of my projects as a replacement for Travis. Have to say that it also has free plan limits but it worked well for me |
Yeah that works too :-) |
Closing since unfortunately this project is no longer maintained. See: |
Resolves #1277