willmcpo / body-scroll-lock

Body scroll locking that just works with everything 😏
MIT License
4.04k stars 337 forks source link

Add "module" field to package.json #93

Closed natemoo-re closed 4 years ago

natemoo-re commented 5 years ago

I just ran into issue #79. The README states that React/ES6 users can use the library as such:

import { disableBodyScroll, enableBodyScroll, clearAllBodyScrollLocks } from 'body-scroll-lock';

While that may be true in some situations, you may need to explicitly reference the es6 file in other cases.

import { disableBodyScroll, enableBodyScroll, clearAllBodyScrollLocks } from 'body-scroll-lock/lib/bodyScrollLock.es6.js';

By adding a module field to package.json pointing to lib/bodyScrollLock.es6.js, tools like Rollup and Webpack can automatically resolve the esm version of this package.

More Information Rollup Wiki Stackoverflow

willmcpo commented 5 years ago

I believe this PR was done previously, but was reverted due to https://github.com/willmcpo/body-scroll-lock/issues/50.

willmcpo commented 5 years ago

I've not had time to re-investigate, if there's a way to do what you want, and also ensure issues like #50 doesn't come again, let me know 👍

RobbieTheWagner commented 5 years ago

module should not overwrite main, not sure what the previous issue was, but this would be awesome to get in!

NomNes commented 5 years ago

@willmcpo This is really necessary. I checked, it solves the import problem #120 . Merge it, please

willmcpo commented 5 years ago

this PR just needs to validate this issue https://github.com/willmcpo/body-scroll-lock/issues/50 doesn't reoccur. There was a reason it was reverted in the past.

Hopefully the previous validation was flawed or there is an approach to overcome what was found before so that we can re-add module into the package.json, because I agree, it's convenient to have it added.

But browser compatibility is important - so if it's really important for your particular case, please feel free to assist in validating again 👍

RobbieTheWagner commented 5 years ago

What needs to be done to validate?

willmcpo commented 5 years ago

IE11 :)

Screen Shot 2019-08-16 at 4 22 37 pm

RobbieTheWagner commented 5 years ago

@willmcpo there is not enough info there. The problem is likely around some specific version of some specific build tool doing something wrong on that one specific day. webpack, rollup, parcel, etc all know to use the module field for imports, so we would need more info on what was broken for that one random case.

willmcpo commented 5 years ago

:) many presumptions there.

The issue was tested before that fix was reverted. I've not capacity to look into this myself right now. But hopefully will get time soon.

RobbieTheWagner commented 5 years ago

@willmcpo I'm happy to look into it, but as I said in my previous comment, I need much more info to know what to test and validate.

It seems like all we know is it was some version of webpack, which built some app that we don't know about the build on, which broke when using CommonJS in IE11. If we had specific cases to check, I could check it.

TrySound commented 5 years ago

@willmcpo You may cut major release and ask users do not use commonjs to build their apps. It's 2019.

FredKSchott commented 4 years ago

Context: someone is unable to use Chakra UI with Snowpack due to this issue: https://www.pika.dev/npm/snowpack/discuss/76

@willmcpo It looks like that user wasn't transpiling their webpack build for IE11, so that's more an issue on their side. That may have been more prevalent problem in 2018, but in 2020 its more or less standard practice now to compile your node_modules packages in your frontend build (CRA, Next.js, and all the bigs ones do this by default by now).

So I think that IE11 issue has solved itself, and I doubt that that user would still have this problem. This was more of an issue on the consumer's end for skipping IE transpilation (it is untenable to pin the entire ecosystem to IE11 until 2025 :).

diachedelic commented 4 years ago
// package.json
{
...
  "main": "lib/bodyScrollLock.min.js",
  "module": "lib/bodyScrollLock.es6.js",
...
}
$ node -p "require('body-scroll-lock')"
{ disableBodyScroll: [Function],
  clearAllBodyScrollLocks: [Function],
  enableBodyScroll: [Function] }

Seems to require fine with node even if module field is added, which I'm sure the build tools are using under the hood...

sleep-promise uses module, and that library looks very stable.

diachedelic commented 4 years ago

@willmcpo I would put to onus on #50 to prove adding module is really a problem - the author did not even say what bundling tool they were using, let alone provide any configuration.

RobbieTheWagner commented 4 years ago

@willmcpo any chance of merging this? If not, should it be closed? It would help many people to have it.

dlong500 commented 4 years ago

@willmcpo It looks like that user wasn't transpiling their webpack build for IE11, so that's more an issue on their side. That may have been more prevalent problem in 2018, but in 2020 its more or less standard practice now to compile your node_modules packages in your frontend build (CRA, Next.js, and all the bigs ones do this by default by now).

So I think that IE11 issue has solved itself, and I doubt that that user would still have this problem. This was more of an issue on the consumer's end for skipping IE transpilation (it is untenable to pin the entire ecosystem to IE11 until 2025 :).

@diachedelic I really don't understand you saying that compiling the entire node_modules folder is the right thing to do as standard practice. Do you realize how much (completely unnecessary) build time that would add to many projects?

The right thing would be for build tools to become intelligent about packages using the right flags in package.json, and for package authors to include both native and transpiled code along with proper package.json flags. That way things would just work without any extra configuration based on the target environment.

In theory such an ecosystem would also allow a package author to omit the transpiled code (as the build tool would automatically transpile it if necessary), but until we get to near zero use of older browsers I don't see the benefit of not including transpiled code (in addition to native code) in package management systems.

diachedelic commented 4 years ago

@dlong500 absolutely, fixed by #172