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

Vite fails to load browser field from engine.io-client when using yarn berry pnp #1979

Closed
3 tasks done
SyntheticGoop opened this issue Feb 11, 2021 · 14 comments · Fixed by #6493
Closed
3 tasks done

Comments

@SyntheticGoop
Copy link

⚠️ IMPORTANT ⚠️ Please check the following list before proceeding. If you ignore this issue template, your issue will be directly closed.

  • Read the docs.
  • Use Vite >=2.0. (1.x is no longer supported)
  • If the issue is related to 1.x -> 2.0 upgrade, read the Migration Guide first.

Describe the bug

When using yarn berry in pnp mode, Vite fails to properly load the package.json browser field from engine.io-client. This results in browser polyfills not loading.

Reproduction

  1. Initialize a new yarn berry repo
  2. yarn create @vitejs/app
  3. Select preset vue-ts
  4. cd into project
  5. yarn add engine.io-client
  6. In src/main.ts add the following lines import client from 'socket.io-client'; client
  7. yarn dev

Load the site in the browser and open the console. You will see an error.

If you add nodeLinker: node-modules to the .yarnrc.yml file, the browser field detection works properly any you will not see errors.

System Info

  • vite version: 2.0.0-beta.67
  • Operating System: Pop!_OS 20.04
  • Node version: 14.15.4
  • Package manager (npm/yarn/pnpm) and version: yarn 2.4.0

P.S. I understand that yarn pnp is sometimes difficult to work with, but since this is a new project should it not try to work with yarn pnp from the get-go?

@nrayburn-tech
Copy link

There are multiple issues for yarn2/pnp that do not affect the other package managers. It isn't that vite isn't trying to support it, it just looks like yarn2/pnp requires special attention that using other package managers don't.
https://github.com/vitejs/vite/issues?q=is%3Aissue+pnp+is%3Aclosed

Just picking a few comments out of the issues with problems around yarn2/pnp.
#304 (comment)
#1423 (comment)

@SyntheticGoop
Copy link
Author

I suppose yarn 2 is the issue in this case. It's not particularly a major problem for me (I'll just unplug whatever's broken or run in node-modules mode). Just wanted to get this out there.

@swandir
Copy link
Contributor

swandir commented Apr 8, 2021

Another repro here: https://github.com/swandir/vite-pnp-browser-field

It demonstrates that the problem only occurs when the dependency remaps nested modules via "browser" field. The problem also affect dependency optimizing specifically. Production bundling works as expected.

dep1 remaps the packages's entrypoint and it works.
dep2 remaps nested module and it doesn't work with dependency pre-bundling.

I also tried to reproduce this within the optimize-deps playground, but my test passes
swandir@0e078e4
What am I missing here?

@mijamo
Copy link

mijamo commented Apr 13, 2021

Is there currently any known workaround this issue?

@swandir
Copy link
Contributor

swandir commented Apr 13, 2021

Adding the package to optimizeDeps.exclude may fix this particular problem, but it seems like there's another issue with socket.io-client

Uncaught SyntaxError: The requested module '/.yarn/cache/socket.io-client-npm-4.0.1-44df794784-9b163f559c.zip/node_modules/socket.io-client/build/index.js?v=888d6738' does not provide an export named 'default'

@mijamo
Copy link

mijamo commented Apr 13, 2021

Thank you. For me the problem is with react-map-gl that loads mapbox-gl from a script only loaded thought the browser field. Vite does not seem to catch that and instead loads the server version which is a no-op module that just returns null.

Adding react-map-gl to exclude does not work because then I get require is not defined from prop-types (a dependency of that package).

@swandir
Copy link
Contributor

swandir commented Apr 13, 2021

I haven't tried, but disabling optimization for all related deps or at all might help. Otherwise, opting out of PnP (via nodeLinker: node-modules) seems to be the only option.

@mijamo
Copy link

mijamo commented Apr 13, 2021

I actually fixed it using an alias for now.

For React mapbox Gl I used this:

{
	find: /(.+)utils\/mapboxgl/,
	replacement: '$1utils/mapboxgl.browser.js',
}

This is basically what the browser field is supposed to do anyways from https://github.com/visgl/react-map-gl/blob/272cb868bbaa30f861040428751088d5610bc607/package.json#L21

Note that if you use SSR then it wouldn't work or you would need to have that alias set dynamically only for the browser.

It's a very hacky solution but well at least it works in the meantime!

@vfonic
Copy link

vfonic commented May 7, 2021

Hey @mijamo,

can you please help me fix the same issue?

This is the error I get when I don't exclude react-mapbox-gl:

 > node_modules/react-mapbox-gl/lib-esm/map.js:59:25: error: Cannot assign to import "accessToken"
59 │                 MapboxGl.accessToken = accessToken;

Could this be in any way related to the fact that I use TypeScript in my project?

I've upgraded react-mapbox-gl to the latest version: 5.1.1

Other dependencies that I have:

  • @types/mapbox-gl@1.13.0
  • mapbox-gl@1.12.0

The same error occurred before I upgraded react-mapbox-gl to the latest version and I also tried fixing it by upgrading @types/mapbox-gl and mapbox-gl, but I get the same error, because the issue is with react-mapbox-gl and so upgrading those other packages doesn't change anything.

This is my first time trying out vite so I have no idea what I can do to fix this?

@mijamo
Copy link

mijamo commented May 8, 2021

I am using react-map-gl, not react-mapbox-gl . You error is different and I don't think it is related to this issue at all. This issue is about the browser field of packages being ignored.

You error is most likely related to typescript and react-map-gl. A quick googling lead me to https://stackoverflow.com/questions/44332290/mapbox-gl-typing-wont-allow-accesstoken-assignment but in any case this is probably something you should ask the devs of the library.

@vfonic
Copy link

vfonic commented May 8, 2021

Thanks @mijamo! I've found the same SO thread, but to no avail. I'll create an issue on react-mapbox-gl repo.
Thanks again!

@swandir
Copy link
Contributor

swandir commented Aug 26, 2021

I've done some debugging on this and stumbled over this place

// explicit resolveDir - this is passed only during yarn pnp resolve for
// entries
if (resolveDir) {
_importer = normalizePath(path.join(resolveDir, '*'))

Later during resolution importer is used to populate idToPkgMap

export const idToPkgMap = new Map<string, PackageData>()

which is used to access closest package manifest to a module in order to resolve browser field mappings.

function tryResolveBrowserMapping(
id: string,
importer: string | undefined,
options: InternalResolveOptions,
isFilePath: boolean
) {
let res: string | undefined
const pkg = importer && idToPkgMap.get(importer)

But the * isn't used here, so in Yarn PnP mode pkg is always undefined.

Does anyone know what was the idea behind this * thing?

Removing this special case for PnP and replacing it with just @yarnpkg/esbuild-plugin-pnp seems to fix the problem of locating pkg. It fixes my repro with simple browser field mappings. But then there seem to be another problem, in this case with deck.gl's dependencies

pnp:.../.yarn/cache/@loaders.gl-loader-utils-npm-3.0.9-63b908e943-f0a4924a19.zip/node_modules/@loaders.gl/loader-utils/dist/esm/lib/node/fs.js:3:9: error: No matching export in "browser-external:util" for import "promisify"
    3 │ import { promisify } from 'util';
      ╵          ~~~~~~~~~

 > pnp:.../.yarn/cache/@loaders.gl-core-npm-3.0.9-f4ee66ebb1-4272a1a434.zip/node_modules/@loaders.gl/core/dist/esm/iterators/make-stream/make-node-stream.js:2:9: error: No matching export in "browser-external:stream" for import "Readable"
    2 │ import { Readable } from 'stream';
      ╵          ~~~~~~~~

Any pointers?

@IanVS
Copy link
Contributor

IanVS commented May 17, 2022

It seems I'm hitting this in the storybook builder for vite as well. I find that in my case, setting pnpMode to loose also resolves the issue, in case that's useful information at all. Here's a reproduction: https://github.com/IanVS/vite-pnp-object-inspect.

@swandir
Copy link
Contributor

swandir commented May 17, 2022

I find that in my case, setting pnpMode to loose also resolves the issue

The code fails here
https://github.com/inspect-js/object-inspect/blob/main/index.js#L68

Because ./util.inspect.js is disabled for the browser here (which Vite fails to handle in PnP mode):
https://github.com/inspect-js/object-inspect/blob/4ec8893ea9bfd28065ca3638cf6762424bf44352/package.json#L74-L76

And inside it a node's builtin util is being imported:
https://github.com/inspect-js/object-inspect/blob/4ec8893ea9bfd28065ca3638cf6762424bf44352/util.inspect.js#L1

Looks like PnP loose mode allows the bundler to pick up the util npm package that is present in your package tree but is not accessible in strict PnP mode because it's not a dependency of object-inspect.

So, cause is the same, but PnP loose mode is only an accidental workaround in this case, which actually might be unwanted behavior.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants