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

Turn into real ES Node module #49

Merged
merged 15 commits into from
Sep 8, 2020
Merged

Turn into real ES Node module #49

merged 15 commits into from
Sep 8, 2020

Conversation

notlmn
Copy link
Contributor

@notlmn notlmn commented Jul 25, 2020

Closes #48

Shamelessly copied code over from https://github.com/fregante/github-url-detection because the state of ES module is a friggin mess right now.

Haven't tested this yet though, but it should work fine.

Sample npm pack:

npm notice 📦  [email protected]
npm notice === Tarball Contents === 
npm notice 1.1kB  license         
npm notice 26.7kB cjs/index.js    
npm notice 26.7kB esm/index.js    
npm notice 22B    esm/package.json
npm notice 1.6kB  package.json    
npm notice 9.1kB  readme.md       
npm notice 3.2kB  cjs/index.d.ts  
npm notice 3.2kB  esm/index.d.ts  
npm notice === Tarball Details === 
npm notice name:          webext-options-sync                     
npm notice version:       1.2.3                                   
npm notice filename:      webext-options-sync-1.2.3.tgz           
npm notice package size:  11.0 kB                                 
npm notice unpacked size: 71.6 kB                                 
npm notice shasum:        2c6cccbf80f18bd5cbc7ac2034d0d8dad3d128b1
npm notice integrity:     sha512-Qlw7e65RTyBUk[...]SA4sWujTi8vdg==
npm notice total files:   8                                       
npm notice 
webext-options-sync-1.2.3.tgz

@fregante
Copy link
Owner

Two issues:

  • I actually had it before and dropped it (probably because my dependency was not a true ESM either) 35eb290
  • Also webext-options-sync-per-domain would also have to be updated to support RGH

If this can be a true ESM, then it should ONLY be ESM following fregante/meta#1

@fregante
Copy link
Owner

fregante commented Jul 25, 2020

I double-checked. 2 out of 3 dependencies are not true ESM so I can’t convert this one either.

The only way to convert would be to change the dependencies or vendor them (😬).

Following fregante/meta#1 also basically means that almost no changes in this PR are applicable.

@fregante fregante closed this Jul 25, 2020
@fregante
Copy link
Owner

Well I’m a bit silly. Rollup solves exactly that problem. So this PR only needs to drop the CJS bundle and just make it ESM by default

@fregante fregante reopened this Jul 25, 2020
@fregante
Copy link
Owner

Can you also un-vendor this dependency? https://github.com/fregante/webext-options-sync/blob/master/vendor/lz-string.js

package.json Outdated Show resolved Hide resolved
rollup.config.js Outdated Show resolved Hide resolved
@fregante
Copy link
Owner

fregante commented Jul 26, 2020

Base: 39kB

npm notice === Tarball Contents === 
npm notice 1.1kB  license     
npm notice 39.6kB index.js    
npm notice 1.6kB  package.json
npm notice 9.1kB  readme.md   
npm notice 3.2kB  index.d.ts  
npm notice === Tarball Details ===       
npm notice package size:  14.1 kB                                 
npm notice unpacked size: 54.5 kB

+ vendored lz-string: 34kB

npm notice === Tarball Contents === 
npm notice 1.1kB  license     
npm notice 34.4kB index.js    
npm notice 1.6kB  package.json
npm notice 9.1kB  readme.md   
npm notice 3.2kB  index.d.ts  
npm notice === Tarball Details ===        
npm notice package size:  13.3 kB                                 
npm notice unpacked size: 49.4 kB                                 

+ minifier: 28kB

npm notice === Tarball Contents === 
npm notice 1.1kB  license     
npm notice 28.0kB index.js    
npm notice 1.6kB  package.json
npm notice 9.1kB  readme.md   
npm notice 3.2kB  index.d.ts  
npm notice === Tarball Details ===         
npm notice package size:  10.8 kB                                 
npm notice unpacked size: 43.0 kB                

It's worth keeping both things as they are. Perhaps one day I'll be able to drop the serializer, but it's not easy... #28 (comment)

@fregante
Copy link
Owner

You can test it in your PR with:

npm install webext-options-sync@next

@fregante fregante changed the title Add true ES module Add ES module Aug 16, 2020
@fregante fregante changed the title Add ES module Turn into real ES Node module Sep 8, 2020
@fregante
Copy link
Owner

fregante commented Sep 8, 2020

Note: WOS is already an ES Module, just not a Node ES Module. This PR is missing type:module, main:index.js

@fregante fregante marked this pull request as draft September 8, 2020 04:48
@fregante fregante mentioned this pull request Sep 8, 2020
@fregante
Copy link
Owner

fregante commented Sep 8, 2020

fregante/webext-options-sync-per-domain#1 also seems to be imported/tested correctly in RG under Node 14

@fregante fregante marked this pull request as ready for review September 8, 2020 05:26
@fregante
Copy link
Owner

fregante commented Sep 8, 2020

Well, everything seems to work! Including #52

The tests still aren't compatible with Node 14, but that can be separate. Related: vadimdemedes/dom-chef#68 (but this package won't have the same problem)

webext-options-sync-per-domain would also need the same thing due to its mem dependency and can't be type:module until then (even if I tested it and it works in RG)

@fregante fregante merged commit 5bb5b61 into fregante:master Sep 8, 2020
@fregante
Copy link
Owner

fregante commented Sep 8, 2020

This will be released as a breaking release. If anyone can show just cause why this module cannot lawfully be published as breaking, let them speak now or forever hold their peace.

@notlmn
Copy link
Contributor Author

notlmn commented Sep 8, 2020

If anyone can show just cause why this module cannot lawfully be published as breaking, let them speak now or forever hold their peace.

I blame Node ESM again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add true ES module
2 participants