Skip to content

Add transformBundle plugin hook - #387

Merged
Rich-Harris merged 1 commit into
masterfrom
transform-bundle-hook
Dec 30, 2015
Merged

Add transformBundle plugin hook#387
Rich-Harris merged 1 commit into
masterfrom
transform-bundle-hook

Conversation

@TrySound

Copy link
Copy Markdown
Member

Need to decide what to do with promises. This does not seem like a good idea.

.then(bundle => {
  return bundle.generate({...});
}).then(({ code, map }) => {})

@Rich-Harris

Copy link
Copy Markdown
Contributor

Ah, good point. I think it's okay to expect bundle transformers to be synchronous, rather than changing the API to be async – after all, if someone really needs to do something asynchronously, this is the one place where they can do that, since it's the last stage in the process.

@TrySound

Copy link
Copy Markdown
Member Author

@Rich-Harris Do you have any test suggestions?

Comment thread src/utils/transformBundle.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module transformers are called with the code from the previous step, rather than a {code, map} object – I think we should stick to that, because it makes transformers a lot more straightforward. Somehow we need to figure out how to compose all the sourcemaps together 😁

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think composing sourcemaps should to transformer. Like with uglify we just pass previous map.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uglify is very rare in that it accepts an input sourcemap. Sourcemaps are bewildering; expecting people to manage that complexity creates a huge burden (worrying about the sourcemaps from your own transformation step is hard enough, let alone someone else's), and in practice would mean that this hook never got used for anything except Uglify.

@Rich-Harris

Copy link
Copy Markdown
Contributor

@TrySound we should probably have some sourcemap tests. Maybe we should go the whole hog and try uglifying code to check the intended use case?

@TrySound

Copy link
Copy Markdown
Member Author

Fix #381

@TrySound
TrySound force-pushed the transform-bundle-hook branch from e786b5b to d16d8aa Compare December 30, 2015 20:53
@TrySound

Copy link
Copy Markdown
Member Author

@Rich-Harris Let's merge this and you can rebase your branch from master.

@Rich-Harris

Copy link
Copy Markdown
Contributor

@TrySound what's the advantage to doing it that way? Both branches have the green light (#391 is based on this branch)

@TrySound

Copy link
Copy Markdown
Member Author

@Rich-Harris Clean commits history is good advantage for me.

Rich-Harris added a commit that referenced this pull request Dec 30, 2015
@Rich-Harris
Rich-Harris merged commit 074a4ec into master Dec 30, 2015
@TrySound
TrySound deleted the transform-bundle-hook branch June 19, 2016 16:37
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.

2 participants