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 #1310

Merged
merged 2 commits into from Jan 17, 2019
Merged

Update Rollup #1310

merged 2 commits into from Jan 17, 2019

Conversation

simonhaenisch
Copy link
Contributor

@simonhaenisch simonhaenisch commented Jan 10, 2019

This updates Rollup to the latest version (1.1.0) which introduced breaking changes (see this PR: rollup/rollup#2293).

The main difference is that Rollup's bundle.generate now returns a different format.

experimentalCodeSplitting is on by default now and has been removed, so I removed it from the Rollup config.

For src/compiler/bundle/write-bundles.ts, I had to map the array into a different format: previously the chunks were returned as an object

{ filename1: 'code', filename2: 'code' }

, but now it is an array of objects:

[ { fileName: 'filename1', code: 'code' }, { fileName: 'filename2', code: 'code' } ]

Not sure whether my .map and .reduce approach is bad performance-wise (I didn't notice a difference for my project)... it might be better to change the signature of the receiving function in order to avoid this transformation?

Closes #1306.

@simonhaenisch
Copy link
Contributor Author

simonhaenisch commented Jan 11, 2019

@adamdbradley I looked into fixing the tests and they all fail with the same error because of an upstream issue of rollup-plugin-node-resolve in browserify/resolve.

E. g. for service-worker.spec.ts, somewhere it tries to resolve ./components/cmp-a/cmp-a.js with /src as the basedir (which might be an error on our part because it's supposed to be a relative ./src? or shouldn't matter because of the in-memory fs?). There is now a fs.realpath check in path.resolve, which assumes that /src/components/cmp-a/cmp-a.js is an absolute path because it starts with a / and thus fails, and then there is some faulty code that passes on undefined as the basedir.

Here is the related code: https://github.com/browserify/resolve/blob/v1.9.0/lib/async.js#L47-L50. It breaks the init(realStart) because realStart is undefined in this case.

I switched out init(realStart) with init(realStart || absoluteStart) and then all the tests pass again.

I now found out that there is already a fix for this in master, but the latest resolve version (1.9.0) was released without it (see browserify/resolve#177 (comment)) because apparently it's a breaking change. I'll try find out there when that fix will be released. I suggested a patch for this upstream issue (browserify/resolve#181), and waiting for it to get released.

@simonhaenisch
Copy link
Contributor Author

simonhaenisch commented Jan 13, 2019

The AppVeyor build fail seems to be unrelated as the same thing fails in other PRs as well (I'm pretty sure it's because of how normalizePath from compiler/util.ts is used).

The karma test fails because it tries to generate a bundle with entry test-app/global.ts which somehow doesn't get transpiled?

Rollup: Parse Error: test-app/global.ts:3:8
Unexpected token (Note that you need plugins to import files that are not JavaScript)

Would be nice if someone could point me in the right direction as to why this happens, as I use a globalScript in my own project as well which compiles successfully. First I thought the issue is that it's not part of the includes in the tsconfig, but that didn't fix it.

@manucorporat
Copy link
Contributor

manucorporat commented Jan 16, 2019

Does rollup uses browserify/resolve? yep i noticed the same problem with undefined, I would be super happy to merge this PR once this issue with undefined is resolved.

Btw, thanks a lot for the PR!

@simonhaenisch
Copy link
Contributor Author

simonhaenisch commented Jan 16, 2019

Does rollup uses browserify/resolve?

Not Rollup itself, but rollup-plugin-node-resolve. I already fixed the upstream issue but waiting for the patch to be accepted and released.

A workaround I found is to pass preserveSymlinks: true to the node-resolve plugin config, which I did in this PR, but I actually want to undo it again because I don't think it should be set(?).

@manucorporat any idea about why Rollup would fail with the globalScript in the karma test? It's like it doesn't accept the .ts extension or something, but I don't understand why because it works with my globalScript in my project.

@adamdbradley
Copy link
Contributor

adamdbradley commented Jan 16, 2019

Ok, I'll dig into this more, thanks

@adamdbradley adamdbradley merged commit dd31c4f into ionic-team:master Jan 17, 2019
@adamdbradley
Copy link
Contributor

thanks!!! I've got some fixes after this merge too that should get everything up to speed.

@bitflower
Copy link
Contributor

Nice, just got it in Stencil 0.16.4

@simonhaenisch simonhaenisch deleted the update-rollup branch January 17, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants