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

added ESM support #140

Conversation

MarkusDeutschmann
Copy link
Contributor

  • added ESM support - dist-esm will be created in addition to dist
  • removed lodash dependency for easier use of ESM
  • because of ESM the static imports needed to be changed - now ending with *.js
  • needed to replace ts-prune with unimported as ts-prune does not supporting ESM imports
  • updated dependencies

removed lodash dependency for easier use of ESM
because of ESM import now ending with *.js
needed to replace ts-prune with unimported as it ts-prune does not support esm imports
updated dependencies
@MarkusDeutschmann MarkusDeutschmann mentioned this pull request Jun 12, 2023
@patik
Copy link
Owner

patik commented Jun 13, 2023

Awesome, thank you! Unfortunately I won't have a proper computer available for the next couple weeks to check it over and publish the new version. Hope it's okay to wait.

module/src/common/options.ts Outdated Show resolved Hide resolved
module/src/async/index.ts Show resolved Hide resolved
@patik
Copy link
Owner

patik commented Jun 14, 2023 via email

@MarkusDeutschmann
Copy link
Contributor Author

Thanks, ok I think it won't break anything to remove lodash.
Sure it ok to wait.
I would be happy to not break something but also provide more functionality.

@patik
Copy link
Owner

patik commented Jun 14, 2023

If the tests are passing on your end, please bump the version number in package.json and I'll merge it. With any luck, the GitHub action will work and automatically publish it to NPM so you don't need to wait for me to get back to my laptop.

updated minor dependencies
@MarkusDeutschmann
Copy link
Contributor Author

MarkusDeutschmann commented Jun 15, 2023

Thanks.
Here are the result of the test:

  • yarn lint:
yarn run v1.22.19
$ yarn workspaces run lint

> withinviewport
$ yarn typecheck && yarn run code-lint && yarn unused-exports
$ tsc --noEmit
$ eslint --ext .ts --fix ./src/**/*.ts
$ unimported --no-cache
✓ There don't seem to be any unimported files.

> @patik/within-viewport-demo
$ yarn typecheck && yarn code-lint && yarn unused-exports
$ tsc --noEmit
$ eslint --ext .ts --fix ./src/**/*.ts
$ unimported --no-cache
✓ There don't seem to be any unimported files.
Done in 10.84s.
  • yarn test:
yarn run v1.22.19
$ yarn --cwd module test
$ yarn jest --coverage
$ C:\git_repo\within-viewport\node_modules\.bin\jest --coverage
 PASS  src/tests/specs/strictCallbackAndRootMargin.test.ts
 PASS  src/tests/specs/options.test.ts
------------|---------|----------|---------|---------|-------------------
File        | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
------------|---------|----------|---------|---------|-------------------
All files   |   96.66 |    89.65 |     100 |   96.55 |                   
 options.ts |   95.45 |       88 |     100 |   95.45 | 47                
 sides.ts   |     100 |      100 |     100 |     100 |                   
------------|---------|----------|---------|---------|-------------------

Test Suites: 2 passed, 2 total  
Tests:       37 passed, 37 total
Snapshots:   0 total
Time:        3.21 s
Ran all test suites.
Done in 6.74s.
  • yarn cover
yarn run v1.22.19
$ yarn --cwd module test --collectCoverage
$ yarn jest --coverage --collectCoverage
$ C:\git_repo\within-viewport\node_modules\.bin\jest --coverage --collectCoverage
 PASS  src/tests/specs/strictCallbackAndRootMargin.test.ts
 PASS  src/tests/specs/options.test.ts
------------|---------|----------|---------|---------|-------------------
File        | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
------------|---------|----------|---------|---------|-------------------
All files   |   96.66 |    89.65 |     100 |   96.55 |                   
 options.ts |   95.45 |       88 |     100 |   95.45 | 47                
 sides.ts   |     100 |      100 |     100 |     100 |                   
------------|---------|----------|---------|---------|-------------------

Test Suites: 2 passed, 2 total  
Tests:       37 passed, 37 total
Snapshots:   0 total
Time:        2.625 s
Ran all test suites.
Done in 5.72s.

But no hurry regarding the release. If you want to check by yourself, I understand that. It's no issue to wait a while.

@patik
Copy link
Owner

patik commented Jun 15, 2023

Thanks a lot. I will wait then, just in case something goes awry.

@patik
Copy link
Owner

patik commented Jun 26, 2023

There were some conflicts with the yarn.lock that needed to be fixed, so I hope it's okay I pushed to your branch.

So far it looks good in terms of passing tests, etc. However I noticed an issue with the tree-shaking. I added this module to another project, imported just { withinviewport }, and ran the Webpack bundle analyzer. It shows that the jQuery plugin is still being included:

CleanShot 2023-06-26 at 22 33 49@2x

Likely this is an issue with the way things are exported from index.ts and is not a problem with your PR. I'll need to look into reconfiguring the exports so that tree-shaking happens properly.

@patik
Copy link
Owner

patik commented Jul 2, 2023

@MarkusDeutschmann I managed to get it working so that both CJS and ESM are in the same dist folder and tree-shaking works fine (e.g. when I use this package in another app, the jQuery plugin doesn't get bundled). Please take a look at the recent changes and you can merge it when you're ready.

patik and others added 3 commits July 2, 2023 14:46
…emoved-lodash-dependency

Merge remote-tracking branch 'origin/main' into add-ESM-support-and-removed-lodash-dependency
postbuild step now also Windiows compatible
@MarkusDeutschmann
Copy link
Contributor Author

Hi @patik Thanks a lot. From my point of view it you change are fine. The only thing that I changed is the postbuild step. I just moved it from the bash script into the package.json script section. Reason is that I would like to prevent the break of local dev builds on Windows systems. I hope this is ok for you.
Changes now merged.

@patik
Copy link
Owner

patik commented Jul 6, 2023

@MarkusDeutschmann Looks good Works well on the Mac too.

Thanks again for the PR!

@patik patik merged commit 511231a into patik:main Jul 6, 2023
@patik
Copy link
Owner

patik commented Jul 6, 2023

Closes #141

@MarkusDeutschmann MarkusDeutschmann deleted the add-ESM-support-and-removed-lodash-dependency branch July 20, 2023 14:34
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.

2 participants