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

Fixes conditional chaining operator issues for webpack 4 users #85

Merged
merged 1 commit into from
May 30, 2022
Merged

Fixes conditional chaining operator issues for webpack 4 users #85

merged 1 commit into from
May 30, 2022

Conversation

gerardosabetta
Copy link
Contributor

Hey there this fixes issue #38.

I need this for my project, hope it also helps folks out there.

@josdejong
Copy link
Owner

Thanks Gerardo, looks good.

Three small remarks:

  1. Is there a specific reason to use getBabelOutputPlugin and not babel? (I hadn't seen it before. Does the source map still work with that transpilation step?)
  2. I just published a new version of svelte-jsoneditor, can you fix the merge conflicts in this PR?
  3. Can you use exact version numbers in the package.json devDependencies?

@gerardosabetta
Copy link
Contributor Author

Hey Jos, yes I will resolve those conflicts. I used getBabelOutputPlugin because I've read the following in the docs:

You can run @rollup/plugin-babel on the output files instead of the input files by using getBabelOutputPlugin(...)

So I might be wrong but I think this makes babel work on the bundled file instead of the src files. I can change it to babel if you want I think it would be less overhead

@josdejong
Copy link
Owner

So I might be wrong but I think this makes babel work on the bundled file instead of the src files. I can change it to babel if you want I think it would be less overhead

Right now the rollup config is only used to generate this single bundled file. The original src files should indeed not be touched, but would rollup really do that otherwise? I would have to do some digging to on the behavior of rollup, I'm not an expert 😅 . Most important is that the outputted bundle works, is transpiled, and that the source map works.

@gerardosabetta
Copy link
Contributor Author

To be honest I haven't tested the source maps I will set a breakpoint and see if they work. I will also compare using babel and getBabelOutputPlugin I'm also not an expert when it comes to rollup 😅

@gerardosabetta
Copy link
Contributor Author

Hey @josdejong I fixed the conflicts. I'm still not 100% sure that my change does not affect the sourcemaps and I'm having trobule testing those can you or someone please help me assert that the sourcemaps are functional? Thanks in advance.

I've also switched to exact version numbers in the package.json

@josdejong
Copy link
Owner

Thanks for the updates.

I tried out with babel instead but can't get that working. So let's just go with getBabelOutputPlugin 😁 .

About the source maps: I just noticed that these are broken since setting up typescript (see #19 (comment)), so there is work to be done on that anyway, and this PR is not introducing a regression in that regard.

Will merge your PR now

@josdejong josdejong merged commit 5ac6a98 into josdejong:main May 30, 2022
@gerardosabetta
Copy link
Contributor Author

Oh I forgot to mention that I've also tried babel and the generated code still had the conditional chaining operator that's why I kept getBabelOutputPlugin instead.

With regards to the sourcemaps thanks for confirming that they weren't working in the first place, I wanted to make sure it wasn't my change that break them.

Thanks for merging the PR Jos! 🥳

@gerardosabetta gerardosabetta deleted the adds-babel branch May 30, 2022 16:55
@josdejong
Copy link
Owner

😂 yeah well I bumped into the same issue I guess.

I've now published your fix in svelte-jsoneditor@0.3.56 (I've disabled the sourcemap for the time being since it's broken anyway). Thanks again!

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

2 participants