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

Enforces require.resolve for loaders #4532

Merged
merged 14 commits into from Nov 4, 2019
Merged

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Aug 31, 2019

What kind of change does this PR introduce? (check at least one)

  • Bugfix

Does this PR introduce a breaking change? (check one)

  • No

Other information:

This PR enforces require.resolve when adding new loaders to the Webpack configuration. Doing this is strongly advised, as otherwise Webpack itself will do the require calls and may return the wrong plugins depending on the hoisting.

I had to add a few missing dependencies (cache-loader and thread-loader) to two plugins, and a few optional peer dependencies (a feature available in Yarn and npm 6.11+) for optional integrations (such as less, sass, etc).

Ref yarnpkg/berry#368

@sodatea
Copy link
Member

sodatea commented Sep 3, 2019

For the record, I'll review this PR later after the server-side issue of the npm registry resolves (npm/cli#224 (comment)).
Until then, all of our users who use npm would still get the peer dependency warnings and there's no way to eliminate them. It would certainly confuse them.

@DRoet
Copy link
Contributor

DRoet commented Oct 27, 2019

Seems like the upstream issue has finally been fixed 😅

@sodatea
Copy link
Member

sodatea commented Oct 30, 2019

Well, seems there's still a bug in the npm cli…

@arcanis
Copy link
Contributor Author

arcanis commented Oct 30, 2019

@sodatea I've updated my PR to exclusively use peerDependenciesMeta (not peerDependencies for now, which will avoid warnings on npm). Cf yarnpkg/berry#531

@sodatea
Copy link
Member

sodatea commented Oct 30, 2019

Cool! I'll try it out

@sodatea
Copy link
Member

sodatea commented Nov 1, 2019

Just finished the tests locally and this PR looks good to me.

It's late in the night so I'll just write some quick notes here. I'll check them when I get up tomorrow. Here's what we need to do next in the Vue ecosystem:

  • cache-loader as optional peer dependency in vue-loader
  • update prettier dependency version in @vue/component-compiler-utils to 1.17+ for pnp compatibility. (need to be very careful with this change because of previous failed attempts caused by bugs in prettier)
  • add pnp-webpack-plugin to the default config in @vue/cli-service

After these changes, a default project scaffolded by Vue CLI pretty much works with yarn 2, except for the postcss (autoprefixer) configuration.

Seems it's because when we refer to a postcss plugin by its name in the postcss configuration field in package.json, the plugin package is then required by postcss-load-config, and yarn takes this as an illegal access because that plugin is not a direct listed dependency?

Do you have any ideas on how to solve this issue?

A postcss.config.js file with an explicit require expression and an additional devDependencies entry may be too much for a default project. We need it to keep simple and clean.
For now, one workaround, from the design perspective, is to make the default postcss config more implicit, by putting it inside the @vue/cli-service package. I'm not sure if such implicitness is good design so I'm a little hesitant. It's certainly a practical approach though.

Any advice would be greatly appreciated :)

@sodatea
Copy link
Member

sodatea commented Nov 4, 2019

Found the root cause. It's not because of postcss config loading, but due to no explicit dependency on autoprefixer.
I think I'll just hide it away for now.

And we will be able to support yarn 2 in the next release! 🎉

@sodatea sodatea merged commit 0a5c79b into vuejs:dev Nov 4, 2019
@vue-bot
Copy link

vue-bot commented Nov 4, 2019

Hey @arcanis, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚

@arcanis
Copy link
Contributor Author

arcanis commented Nov 4, 2019

And we will be able to support yarn 2 in the next release! 🎉

That's awesome! If you ping me when it lands I'll add a test to our Toolchain CI to track potential regressions 😃

Found the root cause. It's not because of postcss config loading, but due to no explicit dependency on autoprefixer.

In this case wouldn't it work if you were to define autoprefixer as a peer dependency (potentially flagged optional if you want to disable the warning)?

@sodatea
Copy link
Member

sodatea commented Nov 5, 2019

That's awesome! If you ping me when it lands I'll add a test to our Toolchain CI to track potential regressions 😃

Yeah sure 😃

In this case wouldn't it work if you were to define autoprefixer as a peer dependency (potentially flagged optional if you want to disable the warning)?

It's in the user's project so I think it's too explicit (novice users may wondering what this dependency actually does, and many have already asked to hide away as many implementation details as possible from their projects). So I'll just load the config in @vue/cli-service internally to avoid confusion.

@sodatea
Copy link
Member

sodatea commented Nov 5, 2019

BTW, is there any equivalent to the --registry command-line argument in Yarn 2?

@sodatea
Copy link
Member

sodatea commented Nov 9, 2019

@arcanis
v4.1.0-beta.0 was released.

Now I should be able to scaffold a project with the default preset and switch it to Yarn 2:

yarn global add @vue/cli@next
vue create -d hello-yarn-2
cd hello-yarn-2
yarn policies set-version v2
yarn
yarn add vue-cli-plugin-pnp -D
yarn serve

(This now fails with an error Error: A package is trying to access another package without the second one being listed as a dependency of the first one, which I believe is due to Yarn not respecting the peerDependenciesMeta field in vue-loader)


But if I want to create a project with yarn dlx -p @vue/cli@4.1.0-beta.0 vue create -d hello-world, the command failed when installing the project dependencies using the command yarn --registry=... (Vue CLI adds this argument to all package manager commands)
Is there any way to work around this or is this feature removed from Yarn 2?


Also, will yarnpkg/berry#531 be backported to Yarn v1?

@arcanis
Copy link
Contributor Author

arcanis commented Nov 9, 2019

the command failed when installing the project dependencies using the command yarn --registry=... (Vue CLI adds this argument to all package manager commands). Is there any way to work around this or is this feature removed from Yarn 2?

It's "renamed". The problem with --registry was that it was only affecting the one command line which was setting it. Next calls (for example to yarn add) won't have this flag, and thus will operate in a different environment. Even funnier, nested calls (for example in postinstall scripts) don't benefit from the command line of the top-most process, making installs potentially incoherent with themselves ...

So instead you have two recommended ways to set the registry (and most configuration values):

  • If the settings are meant to be part of the development environment, just write them into a .yarnrc.yml file. This way they will persist between calls without impairing the developer experience.

  • If they are meant to be enabled only temporarily, add them to the environment. For example: YARN_NPM_REGISTRY_SERVER=http://... yarn install. It's better than the command line option because you can either set it for the duration of the shell session, or just for the lifetime of a single command.

As for "how to know which way to set the registry depending on the Yarn version", I'm afraid I don't have a better answer than calling yarn --version and branching on that ... 🤔

Also, will yarnpkg/berry#531 be backported to Yarn v1?

It seems rather low impact so not a high priority at the moment, but maybe. At the moment I'm really focusing my efforts to ship the v2 this year 🙂

@arcanis
Copy link
Contributor Author

arcanis commented Nov 9, 2019

Also good news! Running yarn serve against Yarn master works fine - the prebuild downloaded via v2 is just a bit outdated, I'll fix that tomorrow.

To test yourself, once you're on the v2, you can run the following command:

$ yarn set version from sources

(yarn set version is the new name for yarn policies set-version, and from sources is one of its new capabilities: it clones master and builds it transparently!)

[edit] The fixed build is now the one pointed by v2 😃

@sodatea
Copy link
Member

sodatea commented Nov 10, 2019

Cool, I'll use the environment variable approach.


One more thing, ts-pnp needs to be republished to the npm registry to make the optional peer dependency work properly under npm cli.

The change will be retroactive, meaning that, in order for you to see the change, you will need to publish a new package version. The information will be populated to the cache the next time the publish happens.

@arcanis
Copy link
Contributor Author

arcanis commented Nov 10, 2019

I just republished ts-pnp 👍

@arcanis
Copy link
Contributor Author

arcanis commented Nov 25, 2019

Hey @sodatea! I've just started to integrate Vue-CLI in our E2E battery, but I have a weird problem: when running yarn dlx -p @vue/cli@next vue create -d my-vue (I fixed the --registry thing), everything seems fine but I end up with a very basic directory that only contains the following:

{
  "name": "my-vue",
  "version": "0.1.0",
  "private": true,
  "devDependencies": {
    "@vue/cli-plugin-babel": "^4.1.0-beta.0",
    "@vue/cli-plugin-eslint": "^4.1.0-beta.0",
    "@vue/cli-service": "^4.1.0-beta.0"
  }
}

Any idea as to what I might be doing wrong? I suppose the init fails somewhere, but I'm not sure why it wouldn't hard-crash ... 🤔

@arcanis
Copy link
Contributor Author

arcanis commented Nov 25, 2019

Nevermind, I found the reason - since the dlx runtime is a different dependency tree than the one that just got generated, the require.resolve call doesn't know how to access the dependency informations when loading the generator plugins. This will be an hairy one, but I think it's on me to find a way to make it work 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants