atlassian-labs / compiled

A familiar and performant compile time CSS-in-JS library for React.
https://compiledcssinjs.com
Apache License 2.0
1.99k stars 67 forks source link

Fix source map behaviour in Parcel and move node_modules compiled extraction to new parcel transformer #1631

Closed JakeLane closed 7 months ago

JakeLane commented 8 months ago

The current behaviour for the compiled parcel transformer does not handle source maps correctly. This PR resolves these issues by using the source maps correctly and introducing a new parcel transformer to handle external compiled deps correctly.

changeset-bot[bot] commented 8 months ago

🦋 Changeset detected

Latest commit: 99d1e20fd5bf069b0f4656393cb1f7860483fc75

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages | Name | Type | | ------------------------------------- | ----- | | @compiled/parcel-transformer-external | Minor | | @compiled/parcel-transformer | Minor | | @compiled/parcel-config | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

dddlr commented 8 months ago

introducing a new parcel transformer to handle external compiled deps correctly

i'm still reading through and parsing through the PR 😅 but what was the reason behind taking some of the functionality of @compiled/parcel-transformer and putting them into a new package @compiled/parcel-transformer-external? is it to avoid bogging down @compiled/parcel-transformer with too much complexity? (or is having a separate package needed for sourcemaps to work?)

JakeLane commented 8 months ago

I've gone through this PR and conceptually it seems fine to me - there's a fair bit of change, and I don't have a huge amount of context, so it wasn't necessarily easily to mentally parse the changes.

The only comment I had when looking through was whether there are is any opportunity to pass existing sourcemaps into some of the Babel methods (generate or transform) that might save some of the manual merging / manipulation after, but you likely already considered that and there were reasons it wouldn't work.

The main goal for the new transformer is to avoid running the compiled babel plugin for external node modules when not required. Bringing in babel context would be a concern for that goal.

I think we can reuse the AST in other babel contexts. I think that's a perf win on the table but I didn't want to make this change more complicated than it already is. I'll create a ticket on the backlog to action when we can.