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

update rollup-plugin-node-resolve to 3.0.0 #12076

Closed
pravi opened this issue Jan 23, 2018 · 9 comments
Closed

update rollup-plugin-node-resolve to 3.0.0 #12076

pravi opened this issue Jan 23, 2018 · 9 comments

Comments

@pravi
Copy link

pravi commented Jan 23, 2018

Do you want to request a feature or report a bug?

feature

What is the current behavior?

Just update rollup-plugin-node-resolve to 3.0.0

$ node scripts/rollup/build.js 
 BUILDING  react.development.js (umd_dev)
Error: options.skip is no longer supported — you should use the main Rollup `external` option instead
    at nodeResolve (/home/praveen/forge/node-react/node_modules/rollup-plugin-node-resolve/dist/rollup-plugin-node-resolve.cjs.js:34:9)
    at getPlugins (/home/praveen/forge/node-react/scripts/rollup/build.js:239:5)
    at createBundle (/home/praveen/forge/node-react/scripts/rollup/build.js:360:16)
    at rimraf (/home/praveen/forge/node-react/scripts/rollup/build.js:435:13)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)


What is the expected behavior?

We'd like to use rollup-plugin-node-resolve 3.0 in debian. We are packaging react as a dependency of gitlab 9.5 (so apt-get install gitlab works to setup a gitlab server). pr that removed this option rollup/rollup-plugin-node-resolve#90

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

It is not dependent on react version, only its build dependency.

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2018

I don’t understand what you are trying to do. React’s build dependencies should not affect React users in any way.

@pravi
Copy link
Author

pravi commented Jan 23, 2018

@gaearon we are building react from source (from github tarballs). Every package in debian main should be built from its source using tools available in debian. We like to keep only one version of a library/tool in debian and we already have a newer version of rollup-plugin-node-resolve (as an extreme measure we can include multiple versions, but we try to avoid it).

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2018

That doesn't sound very sustainable to me. Even if we fix this outdated dependency, eventually we're bound to have more. In npm world, dependencies are meant to be per-package and isolated.

In any case, we're happy to take a PR updating this particular dep. But we won't be working ourselves on this in the near future. The removal of skip version was the reason we stayed on the earlier version. It might be that it's unnecessary now—you're welcome to experiment.

@pravi
Copy link
Author

pravi commented Jan 23, 2018

@Gaeron I agree it is indeed a lot of work. In many cases we send pr upstream to keep the dependencies in sync. But we think the effort is worth to give a language-agnostic uniform interface to managing all applications in the system (just apt for every software). It actually bridging two diverging cultures.

I have been able to build it by just commenting out the skip option, though it prints a lot of warnings now. https://salsa.debian.org/js-team/node-react/blob/master/debian/patches/no-skip-node-resolve.patch I will see if I can improve it and add external option to rollup. I'm not very familiar with rollup, so it may take some time.

@gaearon
Copy link
Collaborator

gaearon commented Feb 5, 2018

I'm going to close as we won't be specifically working on this. But happy to take a PR.

@pravi
Copy link
Author

pravi commented Mar 10, 2018

@gaearon please review/help pr #12352

@juggernaut451
Copy link

Hey @pravi can you mentor on this issue?
as exactly how to repro this bug and what exactly to be fixed?

@pravi
Copy link
Author

pravi commented Sep 1, 2018

@juggernaut451 Just edit package.json file and update rollup-plugin-node-resolve to 3.0 and run build. We need to fix the failure. You can look at the PR linked here to get an idea.

@juggernaut451
Copy link

@pravi can you have a look at
https://github.com/juggernaut451/react/tree/rollup_fixes
it compiled with rollup-plugin-node-resolve 3.3.0

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.

3 participants