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

Pass config to commonjs and make optimizeDep more flexible #1460

Closed
clouds56 opened this issue Jan 9, 2021 · 2 comments
Closed

Pass config to commonjs and make optimizeDep more flexible #1460

clouds56 opened this issue Jan 9, 2021 · 2 comments

Comments

@clouds56
Copy link

clouds56 commented Jan 9, 2021

I'm always frustrated when import a library, I almost have to optimize every single dependency when I try to import them in my code. The dist of dependencies could be packaged by any tools (webpack, tsc, ...), we cannot expect the default config of commonjs would resolve all issues.

commonjsPlugin({
include: [/node_modules/],
extensions: ['.js', '.cjs']
}),

Describe the solution you'd like
I'd like to have more control on single deps (direct or indirect)
I know the resolve/node-resolve would find the dist file of package for me, but I would still like to

  1. Specify which file is the entry of the package, and gives hint the import style it use (amd/commonjs/module), and also whether it has default export.
  2. Ignore some dependencies of this package (just like alias, but scoped to this package). It's not common that some package would require some node specific deps but not to use them when it found environment is browser.
  3. Forward any other configs in config.optimizedDeps.commonjs to rollup.

Vite keeps these information and translate them to commonjs plugin.

Describe alternatives you've considered
I checked again and again for rollup plugins used in optimizing pass, the only hack/workaround I could use is customResolver in aliasPlugin.

function aliasResolve(): Plugin {
  return {
    enforce: 'pre',
    name: 'my:alias-resolve',
    configResolved(config) {
      const checkMap: {[name: string]: { check: string, replacement: string }[]} = {}
      for (const alias of config.alias) {
        if (alias.replacement.startsWith('/@src/')) {
          alias.customResolver = function (id, importer) {
            id = id.replace('/@src/', 'src/')
            // console.log(id, importer)
            const result = this.resolve(id, undefined, { skipSelf: true })
            result.then((r) => {
              if (r == null || r.id.indexOf(id) === -1) {
                console.warn('[my:alias-resolve] cannot find id:', id, importer, r)
              }
            })
            return result
          }
          // console.log('alias:', alias)
        } else if (typeof alias.find === 'string' && alias.find.indexOf('?check=') !== -1) {
          const [find, check] = alias.find.split('?check=')
          const replacement = alias.replacement
          if (checkMap[find] == null) { checkMap[find] = [] }
          checkMap[find].push({ check, replacement })
          alias.find = alias.replacement = find
          alias.customResolver = function (id, importer) {
            const matched = checkMap[find].find(i => {
              return (i.check === '' && importer == undefined) || (importer?.indexOf(i.check) ?? -1) > 0
            })
            if (!matched) {
              console.debug('[my:alias-resolve] check failed:', id, importer, checkMap[find].map(i => i.check))
              return this.resolve(id, importer, { skipSelf: true })
            }
            const updatedId = id.replace(find, matched.replacement)
            console.debug(id, '=>', updatedId, importer)
            return this.resolve(updatedId, importer, { skipSelf: true })
          }
        }
      }
      // console.log(config)
    }
  }
}

Additional context
And use the plugin in alias (workaround)

    alias: [
      // these types only contains emtpy information
      // https://github.com/rollup/plugins/issues/743
      { find: './types?check=package-a', replacement: '__browser-external' },
      { find: './types?check=package-b', replacement: '__browser-external' },
      // these two lines would alias `./types` to empty only when importer is package-a or package-b
      // and leaves `./types` in other packages as is
      { find: './fs?check=package-b', replacement: '__browser-external' },

      // node related unused import
      { find: 'xhr2-cookies', replacement: '__browser-external' },
      { find: 'http', replacement: '__browser-external' },
      { find: 'https', replacement: '__browser-external' },

      { find: 'package-c', replacement: '/@src/mock/c.js' },
    ],

The solution I hope goes like

    deps: [
      { package: 'package-a', alias: {'./types': '__browser-external'} },
      {
        package: 'package-b',
        ignore: ['./types', 'fs'], // this would make transform `require('fs')` to `undefined`
        entry: 'dist/b.esm.js',
        mode: 'esm',
        default: false, // when some package import the default of this package, it would `import * as b from 'b'`
      },
      {
        package: 'package-c', entry: '/@src/mock/c.js' // this would load local src/mock/c.js other than any file in node_modules
      },
      { package: 'http', ignore: true },
    ]
@yyx990803
Copy link
Member

yyx990803 commented Jan 9, 2021

The premise of Vite is that you should use packages that provides native ESM formats as much as possible. By allowing you to configure and use all the packages that ship fundamentally broken code for browsers, we will never move the ecosystem forward.

It's not Vite's goal to be compatible with every package out there.

Some notes on your use cases:

  • Optimizer already respects alias, which means you already can redirect package entries.

  • Packages can use the browser field to ignore modules and Vite already supports that. If a package doesn't specify this but still expect to work, that's really an issue in the package itself. Just because it works in webpack 4 doesn't mean it should work in every other tool.

  • The future of conditionally determining files to use for browsers will likely be resolved via the exports field in package.json. There's already conversation between the Node.js team and authors of popular tools on building a consensus around this. In the future, we hope all modern packages should use exports field to properly define how it should be packaged for the web, without requiring the user to manual configure hacks.

Since you already found workarounds, this will be wontfix.

@clouds56
Copy link
Author

No the commonjs plugin assumes all dependencies are commonjs if no options pass to it.
I do agree that we could use ESM formats as much as possible, but the fact is we could not mix them without pass extra args to commonjs.

Forward any other configs in config.optimizedDeps.commonjs to rollup.

Last but not least, we could not expect commonjs work without hack in every other package, the 2 issues of rollup rollup/plugins#743, rollup/plugins#658 occurs when import every complex package. (even more rollup/plugins#658 is also a workaround that not work in every case).

Some notes in my cases:

  • The alias would drop customResolver when resolve config, we have to encode information in find/replace string and write a new plugin to transform it (see aliasResolve function above)
  • Me an end-user could not take control of every modules I import, especially those which works well in traditional tools.
  • I hope the days exports field accept by every modern package would come, but I think that at least 3 years after, considering that most module doesn't even has browser field now and the modern package vite doesn't have exports field, nor the vue-next
  • I hacks a lot but cannot workaround for everything, and the basic solution is as simple as transport additional configs to commonjs plugin.

So please.

@yyx990803 yyx990803 reopened this Jan 10, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants