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

Revert broccoli asset rev change #298

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

apellerano-pw
Copy link

Fixes #275.

ember-cli-terser 4.0.2 contains a regression documented in #275 which forces people using both ember-cli-terser and broccoli-asset-rev to pin ember-cli-terser to 4.0.1.

One way forward is to revert the change which introduced the regression (PR #266) and release this as 4.0.3.

@st-h
Copy link

st-h commented Mar 19, 2022

🙏 Please, please merge this and find a proper fix for #265. It took me several days to figure out why source maps are broken and that it is caused by a regression with ember-cli-terser

@NullVoxPopuli
Copy link

NullVoxPopuli commented Mar 20, 2022

is there any way you could add a test for this so the regression doesn't re-introduce itself?

Though, I guess because #266 didn't contain any tests, we can't be sure that it was actually fixing anything either.

I'm in favor of merging this PR and releasing 4.0.3, and we really need tests added to cover this stuff before any additional changes around addon load order are done.

@NullVoxPopuli
Copy link

cc @rwjblue and @kellyselden ? idk who to tag 😅

@NullVoxPopuli
Copy link

@st-h can you describe what sort of sourcemaps you are expecting?

  • hi-fi pre-babel sourcemaps?
  • or just "good enough" pre-terser sourcemaps?

@st-h
Copy link

st-h commented Mar 23, 2022

@NullVoxPopuli the problem is that source maps usually contain a sources entry that contains the different classes of the application like myApp/routes/application. After the changes made in #266 the sources entry only contains one link to the minified js file like assets/myapp-<fingerprint>.js. So the source maps just link to the minified js, and therefor won't work.

@NullVoxPopuli
Copy link

is it feasible for you to just use the older version?

The thing is that #266 fixed issue #265, so to revert #266 will re-break #265.

@st-h
Copy link

st-h commented Mar 24, 2022

@NullVoxPopuli Yes, both. Sure it is possible to just stick to the old version if one requires working source maps. However, I am quite puzzled why the author of #266 did not run into that issue. Given that there are quite a few users that commented on #275, I can't be the only one having those issues. I think reverting #266 would be a good idea as other users very likely will run into the same issue and it is quite difficult to track down which addon in which version causes it. (As I already mentioned, it took me days of trial and error) We certainly should be looking for a fix for #265 that does not render source maps useless. But at this point I think keeping things the way they are and hoping users that are affected by this regression will figure things out one day or the other is the worst option that we have.

Also, I wonder what did you do to resolve the issue, as you have also been commenting on #275? Might be there is a workaround none of us are aware of, which also worked for the author of #266 without knowing about it. There probably is some sort of configuration or something that does not trigger the regression.

Pretty much the question at this point is: Do we want accurate line numbers, while source maps do not work at all for some users, or do we want incorrect line numbers for some users while source maps work for all users? Ideally we would have both, but at the moment I don't know where to start to tackle this issue and I pretty much have no time available at all.

@st-h
Copy link

st-h commented Apr 23, 2022

@NullVoxPopuli any updates? What was your workaround to fix this issue in your codebase?

@NullVoxPopuli
Copy link

I think at this point we just have two versions folks use depending on the problem the users are facing. Until someone understands and can debug and fix both problems at the same time, that's how it's gotta be for now (imo, of course). The goal is to move to embroider, and ember-cli-terser is not used under embroider, so an option is to do that migration (I think, I could be wrong).

The main problem here is that different people want different levels of fidelity in production. Ultimately though, everyone should be comfortable with reading the "good enough" output in production. Because even outside of ember, that's sometimes all you can ask for.

@yads
Copy link

yads commented Jan 5, 2023

If different people want different behaviour, perhaps this should be a config instead of forcing people to pin an older version?

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.

Sourcemaps + fingerprinting regression with ember-cli-terser 4.0.2?
4 participants