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

Plugin Alias v3 throws error #203

Closed
mattpilott opened this issue Feb 8, 2020 · 29 comments · Fixed by #426
Closed

Plugin Alias v3 throws error #203

mattpilott opened this issue Feb 8, 2020 · 29 comments · Fixed by #426

Comments

@mattpilott
Copy link

  • Rollup Plugin Name: Alias
  • Rollup Plugin Version: 3.0.1
  • Rollup Version: 1.31.0
  • Operating System (or Browser): macOS 10.15.3
  • Node Version: 13.8.0

How Do We Reproduce?

https://repl.it/repls/WatchfulUnsightlyStruct

Expected Behavior

Expect to compile as per v2.2.0

Actual Behavior

Throws mysterious error (plugin alias) TypeError: Cannot read property 'input' of undefined

Thanks!

@bennypowers
Copy link
Contributor

bennypowers commented Mar 30, 2020

I'm able to avoid this error with:

plugins: [
    customResolver,
    alias({ entries: importAliases, customResolver })
]

As a user, I may have expected alias to cover all cases of resolve

@mattpilott
Copy link
Author

@bennypowers It seems that your workaround is good, will shout if i run into anything.

Thanks!

@pkit
Copy link

pkit commented Apr 23, 2020

It's not a workaround, because it just disables aliasing in a general case.
Alias should come before node-resolve.
Example in README changed to one that actually woks:

// rollup.config.js
import alias from '@rollup/plugin-alias';
import resolve from '@rollup/plugin-node-resolve';

const customResolver = resolve({
  extensions: ['.mjs', '.js', '.jsx', '.json', '.sass', '.scss']
});
const projectRootDir = path.resolve(__dirname);

const config = {
  // ...
  plugins: [
    alias({
      entries: [
        {
          find: 'src',
          replacement: path.resolve(projectRootDir, 'src')
          // OR place `customResolver` here. See explanation below.
        }
      ],
      customResolver
    }),
    resolve()
  ]
};
customResolver.buildStart(config);
export default config

@shellscape
Copy link
Collaborator

@pkit thanks for confirming. We're going to close citing that comment. If the example in the readme doesn't work for anyone else, please drop a comment here.

@vladshcherbin
Copy link

Example from docs doesn't work. Workaround from #203 (comment) works, even if @rollup/plugin-node-resolve is placed after alias:

const nodeResolve = resolve()

plugins: [
  alias({
    entries: { ... },
    customResolver: nodeResolve
  }),
  nodeResolve
]

However, it feels more like a hack than a solution.

@pkit
Copy link

pkit commented Apr 23, 2020

Workaround doesn't work for any node_modules alias.
I.e. my case: use 'react-dom': 'react-dom/profiling' alias
With or without alias the output is the same if placed after node-resolve

@shellscape
Copy link
Collaborator

@vladshcherbin previous commenter indicates that's not so. unless someone can pinpoint why it's not working a positive assertion that it does work is going to take precedent.

@vladshcherbin
Copy link

@pkit didn't test it with node_modules alias, only local ones.

Anyway, this issue shouldn't be closed as it's clearly not working for many users following example from current docs. This is the reason we all came to this issue 😉

@shellscape
Copy link
Collaborator

I'll reopen if someone can provide an updated reproduction. The repro provided on the original post doesn't run.

@pkit
Copy link

pkit commented Apr 23, 2020

@shellscape it's easy to pinpoint why it's not working: when placed after node-resolve alias is working on resolved paths and not on original. i.e. react-dom?commonjs-proxy
But I'm nor sure I need to tell it to you. After all I'm not the author of rollup

@shellscape
Copy link
Collaborator

@pkit I'm not the author of Rollup or the alias plugin either. I'm a contributor and team member for maintaining the plugins. Because of the volume of issues we get here we just need reproductions so we don't spend time on trying to recreate the issues folks report.

@pkit
Copy link

pkit commented Apr 23, 2020

@shellscape cool, anyway. It seems like combination of node-resolve and commonjs makes it not resolvable in a generic case.
I suppose adding more hacks to alias can help.
here:

typeof options.customResolver!.resolveId === 'function'

 else if (
        typeof options.customResolver === 'object' &&
        typeof options.customResolver!.resolveId === 'function'
      )

@vladshcherbin
Copy link

@shellscape here's reproduction - https://github.com/vladshcherbin/rollup-alias-error, latest alias version (3.1.0), docs example. Run with yarn build (or npm)

Screenshot

image

@pkit
Copy link

pkit commented Apr 23, 2020

You know what, it looks like a rollup bug.
buildStart() should run before resolveId() but it looks like that it runs after.
Otherwise there won't be an error in the first place, no matter the order

@pkit
Copy link

pkit commented Apr 23, 2020

@vladshcherbin it all comes to that issue again rollup/rollup#2826
Really...

@mattpilott
Copy link
Author

https://repl.it/repls/AttachedWhimsicalDehardwarization

Take a look in here, head into src/main.js and switch the two import lines to see the issue

pkit added a commit to pkit/plugins that referenced this issue Apr 23, 2020
@shellscape
Copy link
Collaborator

Thanks!

@shellscape shellscape reopened this Apr 23, 2020
@jasonkarns
Copy link

jasonkarns commented May 6, 2020

Also reproduces when customResolver is at the entry level:

  plugins: [
    alias({
      entries: [{
        find: '@optimizely/optimizely-sdk',
        customResolver: resolve({browser: false})
      }]
    }),
    resolve({ browser: true })
  ]

@lukastaegert
Copy link
Member

What needs to happen is that node-resolve should keep the resolveId hook in a pending promise state until its own buildStart hook has run. Adding more hooks will not really solve anything, just add more combinatorics to make issues less likely and more complicated. I think that case is an important gotcha that we should also add to the official Rollup docs

@shellscape
Copy link
Collaborator

@lukastaegert that's a little outside of my wheelhouse. would you be able to POC that? I might be able to take it from there.

@lukastaegert
Copy link
Member

Actually on second thought I think there might be a much simpler solution that would be inside Rollup and not require any plugin to change: We just make sure that this.resolve and similar plugin context functions do not trigger any hooks synchronously. Then as all buildStart hooks are started together, each plugin will have a chance to pick up Rollup's option before any other hook is triggered for the first time. Will try it out later and publish a fix if this works out as I think.

@pkit
Copy link

pkit commented May 29, 2020

What needs to happen is that node-resolve should keep the resolveId hook in a pending promise state until its own buildStart hook has run. Adding more hooks will not really solve anything, just add more combinatorics to make issues less likely and more complicated. I think that case is an important gotcha that we should also add to the official Rollup docs

Yup. That's what I did in the end. It also kind of ugly looking. But better.

@lukastaegert
Copy link
Member

Ok, the problem is quite a different one than I thought, it is in a way what was already posted here: #203 (comment)
The idea of just running a rollup plugin outside Rollup, i.e. node-resolve just used as a customResolver, is a rather bad one, because the buildStart hook cannot be triggered for that plugin. And that is not surprising if the plugin is not used as a Rollup plugin but just as a function.

At the moment, this only concerns the buildStart hook, so what the plugin should actually do I guess is in its own buildStart hook to check if any custom resolvers have a buildStart hook and run them manually.

@lukastaegert
Copy link
Member

I can try to set up a PR.

@lukastaegert
Copy link
Member

fix at #426

@bennypowers
Copy link
Contributor

For the record, I resolved this problem once and for all by compiling aliases out-of-band using ttsc. This also bought me a nice build-time reduction.

@pkit
Copy link

pkit commented May 31, 2020

@bennypowers can you expand on that? What exactly was compiled?

@bennypowers
Copy link
Contributor

@pkit sure thing. This solution will only work for users writing typescript sources who are willing to run a wrapper around tsc. It's not a solution to this thread's core problem. I'm only posting it here to help my future self people who land here from google

So my use case is to alias module like

import '#components/card`

instead of

import '../../../card/card.ts

I had been:

  • running @rollup/plugin-typescript to compile and build at the same time
  • using @rollup/plugin-alias to resolve module specifiers that start with #

There were two downsides:

  1. builds were taking 2-5 minutes
  2. I had to maintain my module aliases both in tsconfig.json's paths field, as well as in rollup config

My ultimate solution was to use ttypescript with this in tsconfig:

{
  "compilerOptions": {
    "plugins": [
      { "transform": "@zerollup/ts-transform-paths" }
    ]
}

As well as doing a two-step build:

  1. compile ts sources using ttsc (no typo)
  2. run rollup on the compiled javascript files.

Those steps solved both the performance (builds down to 30-40 seconds) and maintenance (aliases handled exclusively by typescript) problems.

@pkit
Copy link

pkit commented May 31, 2020

@bennypowers cool. Thanks!
Unfortunately my use case is totally dynamic (I don't need aliases all the time, only for specific build options).
And if my builds were 2 min I would shoot myself anyway... :)

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

Successfully merging a pull request may close this issue.

8 participants