Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

[BREAKING] Remove options.skip #90

Merged
merged 6 commits into from
Apr 1, 2017
Merged

[BREAKING] Remove options.skip #90

merged 6 commits into from
Apr 1, 2017

Conversation

Rich-Harris
Copy link
Contributor

The skip option is confusing and (as far as I can tell) unnecessary. People are better off using the external option in the main Rollup config. Prompted by Rich-Harris/packd#2, I'm toying with the idea of removing it. Any objections?

@Rich-Harris Rich-Harris merged commit fa4fdbc into master Apr 1, 2017
@Rich-Harris Rich-Harris deleted the remove-skip branch April 1, 2017 04:25
@SnareChops
Copy link

@Rich-Harris I understand the removal of this feature, unfortunately it seemed the only option that worked for our project to remove this suborn dependency from the bundles.

tl;dr; - How can I replicate the skip behaviour now that it has been removed?

The dependency is autobahn which is "supposed" to be compatible with browser + NodeJS, however is not rollup friendly and really nasty things happen during bundling when it is included.

The scenario is a little tricky. The frontend and backend of our application both share a common web socket connection library (internally written by us), which is awesome, however the import of autobahn causes issues for the frontend application when bundling. The import * as autobahn from 'autobahn'; is required by the backend NodeJS application to run. We write in typescript and export straight to es5 commonjs. This works great.

In the frontend we use the same source files. So I reference the cjs output of the library in my frontend typescript + angular code. However actually importing the autobahn library is not an option in the frontend because it requires crypto which is a built in node module. Yes there are polyfills available for it, but the autobahn library actually does the right thing here and provides a browser compatible file for the browser that can be loaded globally. While global is not ideal, it works...

This was a nightmare to configure in SystemJS and unfortunately is in rollup as well. In the rollup@^7.1.0 version I had the following which worked. external: ['autobahn'] in the rollup options, global: { autobahn: 'autobahn'} in the write options, and skip: ['autobahn'] in the node-resolve options. This finally worked and we've been running that stack for months without issue.

Full 7.1.0 config:

public rollup(entryFile: string, destFile: string, minified: boolean, useStrict: boolean): Promise<void>{

		return rollup.rollup({
			entry: entryFile,
			context: 'window',
			external: ['autobahn'],
			plugins: this.getPlugins(minified)
		})
			.then((bundle: any) => {
				return bundle.write({
					format: 'iife',
					moduleName: 'oryxModule',
					sourceMap: true,
					useStrict: useStrict,
					globals: {
						autobahn: 'autobahn'
					},
					banner: `/* Auto-generated bundle created by Oryx \n * DO NOT MODIFY CODE IN THIS FILE \n * CODE WILL BE OVERWRITTEN */`,
					dest: destFile
				});
			});
	}

	private getPlugins(minified: boolean): any[]{
		const plugins: any[] = [
			bowerResolve(),
			nodeResolve({jsnext: true, module: true, browser: true, skip: ['autobahn']}),
			commonjs({
				include: 'node_modules/**',
				exclude: [
					'node_modules/autobahn/**'
				],
				namedExports: {
					'node_modules/falcor/lib/index.js': ['Model'],
					'node_modules/@angular/core/bundles/core-testing.umd.js': ['async', 'ComponentFixture', 'resetFakeAsyncZone', 'fakeAsync', 'tick', 'discardPeriodicTasks', 'flushMicrotasks', 'TestComponentRenderer', 'ComponentFixtureAutoDetect', 'ComponentFixtureNoNgZone', 'TestBed', 'getTestBed', 'inject', 'InjectSetupWrapper', 'withModule'],
					'node_modules/@angular/compiler/bundles/compiler-testing.umd.js': ['TestingCompilerFactoryImpl', 'TestingCompilerImpl', 'platformCoreDynamicTesting', 'MockSchemaRegistry', 'MockDirectiveResolver', 'MockNgModuleResolver', 'MockPipeResolver'],
					'node_modules/@angular/platform-browser-dynamic/bundles/platform-browser-dynamic-testing.umd.js': ['platformBrowserDynamicTesting', 'BrowserDynamicTestingModule'],
					'node_modules/@angular/platform-browser/bundles/platform-browser-testing.umd.js': ['platformBrowserTesting', 'BrowserTestingModule'],
					'node_modules/@angular/router/bundles/router-testing.umd.js': ['SpyNgModuleFactoryLoader', 'setupTestingRouter', 'RouterTestingModule']
				}
			}),
			json()
		];

		if(minified){
			plugins.push(uglify());
		}
		return plugins;
	}

Then strikes the rxjs 5.2.0 Unexpected token issue... We downgraded the rxjs to 5.1.1 and decided to see if a patch came around. Now 5.3.0 is out and it was still not working for us, so time to upgrade rollup to the latest 8.x.x, also updating all of the dependencies. Seeing that skip is removed and also that we have external: ['autobahn'] already set, but broken bundles, well rather the bundle succeeds, but at runtime: Uncaught ReferenceError: module is not defined at

module.exports = require('./lib/autobahn');
var index$2 = Object.freeze({});
var autobahn$1 = ( index$2 && undefined ) || index$2;

This wasn't an issue when skip was present. The config used when this error occurred is identical to the previous, except skip: ['autobahn'] has been removed.

Happy to provide more information as this seems to be a small edge case, but if you know of a way to mimic the behaviour of skip so as not to include autobahn in the bundle at all (only the reference to the global) then that would be good to know.

Thanks for your time, and sorry for the extremely long post, but it is a very specific scenario. The rest of rollup has been great and "just works™" out of the box.

@jharris4
Copy link

jharris4 commented Apr 4, 2017

I'm encountering a similar issue. I'm using the skip option to deal with a pesky package (typed-immutable) that is built on top of Facebook's immutable package.

I have immutable specified in the array of packages passed to rollup's main external option, but all 142 KB of immutable's source are output into my bundle unless I also put immutable in options.skip.

To the best of my knowledge, this is something to do with how typed-immutable is importing the immutable library. This is a blocker for me unless I can find some kind of workaround...

@jharris4
Copy link

jharris4 commented Apr 4, 2017

I just created a stripped down example of using rollup to package a library that imports typed-immutable and immutable.

It currently outputs immutable.js to the bundle despite it being in the external option. Any help/suggestions on how to workaround this would be greatly appreciated.

https://github.com/jharris4/typed-immutable-rollup

@SnareChops
Copy link

@Rich-Harris Looks like this is due to #72

@BryanCrotaz
Copy link

How do you skip modules without skip?

Eg AWS Lambda has built in modules available (e.g. aws-sdk)

{
			entry: lambdaPath+'/index.js', 
			dest: 'index.js',
	//		format: 'cjs',
			plugins: [
				rollupResolve({
					customResolveOptions: {
						external: [
							'aws-sdk',
							'cfn-response',
							'crypto'
						]
					}
				}),
				rollupCommonjs({
					exclude: [
						'node_modules/aws-sdk/**'
					]
				}),
				rollupJson()
			]
		}

doesn't work

@DanielJoyce
Copy link

external is ignored currently by this plugin.

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

Successfully merging this pull request may close these issues.

6 participants