Fix TS Build Target - Compile Nullish Coalescing for broader Webpack/Babel/Node support

[monsonjeremy]

Fixes: #883

This PR is meant to fix some issues that exist with the current version of the built code. Using ES2018 as the build target will ensure that no ?? syntax will make it into the build output. Unfortunately this syntax does not have broad support for packages like webpack@4 and other bundlers. I've run into these issues running my tests in node using js-dom as well.

The impact should be low here (bundle size may increase a slight bit) but will make the library much easier to use for people using next.js and create-react-app as well as people with highly custom webpack setups and those who have not made it to webpack 5 yet.

[cosmidumi]

@PaulLeCam can we merge this?

[daftcoder]

@PaulLeCam sorry for bothering, but could it get merged?

[tony-stark-edith]

+1

[monsonjeremy]

Given this is taking a long time I'll point people to a version of the package I released with these changes.

https://www.npmjs.com/package/@monsonjeremy/react-leaflet

npm install --save @monsonjeremy/react-leaflet

Use at your own risk as I will not be further maintaining this package it is only to hold over until this PR gets merged.

[amaster507]

Is there any ETA for this? I tried to use the alternative package (thank you @monsonjeremy) but I am unable to do that because I have other packages depending on react-leaflet which then use the wrong dependency when installed. I have literally been fighting this all day. This is such a simple fix and is so easy to review that a simple merge and patch should be really quick and effortless. Is @PaulLeCam the only one with write access to this repo? This is concerning on the delay of this.

Alternatively, is there an easy way to redirect package dependencies without forking every package myself and modifying the imports?

[monsonjeremy]

@amaster507 Can you use alias from webpack:

https://webpack.js.org/configuration/resolve/#resolvealias

To force other libraries to resolve to my version?

[amaster507]

@monsonjeremy Thank you! I was not aware of this, figured there had to be a way somehow.

npm install @react-leaflet/core@npm:@monsonjeremy/react-leaflet-core
npm install @monsonjeremy/react-leaflet-core
npm install react-leaflet@npm:@monsonjeremy/react-leaflet

And I am up and running! I had to install your core package twice (under both the original name and alias) so that all dependencies are working. ☕ to you!

[amaster507]

@PaulLeCam can you please consider merging this fix!? Using the above work-arounds causes additional problems with other packages that use the wrong context file imports. See my referenced issue above this comment.

@monsonjeremy Thank you so much for your help again! I at least have a working leaflet map again even if I can't use my Google layers right now. Do you have any thoughts on the above workaround with packages that are then calling the wrong context files?

[adeelibr]

@PaulLeCam vote for merging this PR if it helps in resolving the issue.

[egirshov]

@monsonjeremy are you sure you even need to downgrade build target? I get the the build output without any ?? simply running yarn && yarn build:core && yarn build:react-leaflet from latest master/v3.2.0 tag

[monsonjeremy]

@egirshov That may be true, but whatever version is released to npm has the ?? in the built code. It could be that your local environment is having an effect on that, or that @PaulLeCam did a publish with some unintended changes that caused that to end up in the build.

[egirshov]

True, but in case the build somehow depends on the local environment (despite of yarn.lock), I believe the proper fix would be to eliminate that hidden dependency. And in case there was a broken build/publish the solution would be simply publish new version.

[dominikbucher]

@PaulLeCam Also voting for merging this PR. We are using the react-leaflet library similarly with react-scripts and this would make configuration much easier. Thanks a lot for your work!

[BowgartField]

Hi huys,
I'm trying to use https://www.npmjs.com/package/react-leaflet-markercluster library and i have the same problem:
Error: No context provided: useLeafletContext() can only be used in a descendant of <MapContainer>

I tried the @monsonjeremy's version but it didn't work.
There is my package.json:

Thanks you so much for replies !

[monsonjeremy]

@BowgartField This is an unrelated issue to this PR. I recommend you open an issue with that library.

[Matthewnie]

Can we please get this merged?

[bquay]

Would love to have this merged!

[melonwannajack]

Is something blocking this PR ?

[jorgeig-space]

One more vote for this PR. Not sure why @PaulLeCam is not willing to merge this.

[felixb101]

Tried to use @monsonjeremy/react-leaflet-core in combination with react-leaflet-heatmap-layer-react-leaflet-3 but I get errors extending L.Layer:`TypeError: Cannot read properties of undefined (reading 'pane')

getPane(): HTMLElement {

188 | return super.getPane() ?? this._map.getPanes().overlayPane
`

[PaulLeCam]

As already replied in similar previous PRs, if the tools you're using don't support ES2019, that's up to you, please don't force your setup constraints on this library.

[GGAlanSmithee]

@PaulLeCam that line of reasoning is just objectively wrong

Surely you, as a library author, can see the soundness of fixing this problem at the source and not force this burden on every consumer of your library. There is a de-facto consensus in the npm ecosystem in regards to this.

Actually, I made the same argument as you back in 2015 when es modules were gaining traction, but was rebutted by Rich Harris (rollup, Svelte etc) who made the exact same argument that I and other have come to realize to be true:

It puts the burden of worrying about these things on the consumers of libraries, rather than a) the library authors and b) the toolmakers, and it doesn't let you do anything that you can't already do via a standardised build configuration

Please reconsider your stance.

[worc]

Was this ever fixed? This cropped up again in 4.0 and causes problems. Especially in situations where you're not in direct control of your babel transpile step. (Think large enterprise codebase and another team in another timezone on its own schedule and roadmap makes these decisions).

[acrabb]

bump

[felixb101]

Yes, it works now Thank you Felix

…

On Mon, Aug 22, 2022 at 2:39 PM André Crabb ***@***.***> wrote: bump — Reply to this email directly, view it on GitHub <#885 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5AL56IZYYJA5GZQFDXS4TV2PCNPANCNFSM45FEHILQ> . You are receiving this because you commented.Message ID: ***@***.***>

[acrabb]

Hey all, I've been looking at this the past couple days, and its still breaking.

Unfortunately the browse list change didn't work for me.
I installed '@babel/plugin-proposal-nullish-coalescing-operator and added it to the plugins array, and also no luck.

Any other solutions I missed that I should look at? Thanks