Skip to content

Conversation

@PabloSzx
Copy link
Contributor

@PabloSzx PabloSzx commented Jun 8, 2021

Description

ESM Support

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Test Environment:

  • OS: Windows & macOS
  • NodeJS: v14.17.0 & v16.3.0

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@PabloSzx PabloSzx requested a review from dotansimha June 8, 2021 22:28
@changeset-bot
Copy link

changeset-bot bot commented Jun 8, 2021

🦋 Changeset detected

Latest commit: 2270f77

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@theguild-bot
Copy link
Collaborator

theguild-bot commented Jun 8, 2021

The latest changes of this PR are not available as alpha, since there are no linked changesets for this PR.

@PabloSzx PabloSzx force-pushed the esm branch 2 times, most recently from e30907e to ab2ecc7 Compare June 11, 2021 00:11
@vercel
Copy link

vercel bot commented Jun 11, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/theguild/envelop/Bo3FnuYqYotAYJCG5EYMmEZvWMGT
✅ Preview: https://envelop-git-esm-theguild.vercel.app

@dotansimha
Copy link
Member

dotansimha commented Jun 13, 2021

Thank you @PabloSzx !
Can you please help with some instructions for how to test this?

@PabloSzx
Copy link
Contributor Author

PabloSzx commented Jun 13, 2021

@dotansimha I can add an example in another PR that uses esm, but without an example is basically checking if neither the source code and .mjs files don't have any "require", that includes require(, require.resolve, require.cache etc

@dotansimha
Copy link
Member

@dotansimha I can add an example in another PR that uses esm, but without an example is basically checking if neither the source code and .mjs files don't have any "require", that includes require(, require.resolve, require.cache etc

I see, thank you!

Maybe you can add as part of this PR, an example add under /examples/with-esm that uses the built ESM? Just for reference for developers, and I can use that as a base for testing.

@PabloSzx
Copy link
Contributor Author

PabloSzx commented Jun 13, 2021

@dotansimha I can add an example in another PR that uses esm, but without an example is basically checking if neither the source code and .mjs files don't have any "require", that includes require(, require.resolve, require.cache etc

I see, thank you!

Maybe you can add as part of this PR, an example add under /examples/with-esm that uses the built ESM? Just for reference for developers, and I can use that as a base for testing.

There is something tricky to have in mind, if we want the example to work as a real world example or an example as integration testing that imports every plugin to check if ESM doesn't break

@dotansimha
Copy link
Member

@dotansimha I can add an example in another PR that uses esm, but without an example is basically checking if neither the source code and .mjs files don't have any "require", that includes require(, require.resolve, require.cache etc

I see, thank you!
Maybe you can add as part of this PR, an example add under /examples/with-esm that uses the built ESM? Just for reference for developers, and I can use that as a base for testing.

There is something tricky to have in mind, if we want the example to work as a real world example or an example as integration testing that imports every plugin to check if ESM doesn't break

If possible, I would like to see this example as a solution for both, what do you think? is it something we can have here? I mean, just HTTP+Helix+Envelop and ESM configured.

@PabloSzx
Copy link
Contributor Author

If possible, I would like to see this example as a solution for both, what do you think? is it something we can have here? I mean, just HTTP+Helix+Envelop and ESM configured.

I don't think it makes sense for a real world example to import every plugin in the monorepo

@PabloSzx
Copy link
Contributor Author

PabloSzx commented Jun 13, 2021

and the other thing that applies to some of the other projects, just running an example with ESM doesn't ensure that everything works as expected, since the example would have to run everything since a require could be inside a function, a manual check of no "require"s in the source code and mjs files is required and I did in all the PRs I made, and the ones that are set as "draft pr" I'm waiting for a response of my comments since they will require some refactors that I would say are outside of the scope of just "ESM support"

@Urigo
Copy link
Collaborator

Urigo commented Jun 13, 2021

@PabloSzx I think a single simple example of both ESM and CommonJS, not testing every plugin, for each repo, would give us enough confidence.
Later on if we want we can always increase the scope

@theguild-bot
Copy link
Collaborator

theguild-bot commented Jun 15, 2021

✅ Benchmark Results

     ✓ no_errors
     ✓ expected_result

     checks.........................: 100.00% ✓ 250668      ✗ 0     
     data_received..................: 968 MB  16 MB/s
     data_sent......................: 50 MB   829 kB/s
   ✓ graphql_context................: avg=10.58µs  min=6.9µs    med=8.7µs   max=14.66ms p(90)=10.9µs  p(95)=18.4µs 
   ✓ graphql_execute................: avg=137.05µs min=92.1µs   med=122.5µs max=13.58ms p(90)=151µs   p(95)=167.8µs
   ✓ graphql_parse..................: avg=7.74µs   min=5µs      med=6.6µs   max=8.49ms  p(90)=7.8µs   p(95)=10.8µs 
   ✓ graphql_validate...............: avg=39.6µs   min=28µs     med=34.3µs  max=14.01ms p(90)=45.8µs  p(95)=54µs   
     http_req_blocked...............: avg=4.74µs   min=1.1µs    med=1.8µs   max=19.93ms p(90)=2.6µs   p(95)=3.3µs  
     http_req_connecting............: avg=75ns     min=0s       med=0s      max=1.89ms  p(90)=0s      p(95)=0s     
   ✓ http_req_duration..............: avg=4.15ms   min=351.81µs med=3.08ms  max=45.53ms p(90)=8.85ms  p(95)=11.99ms
       { expected_response:true }...: avg=4.15ms   min=351.81µs med=3.08ms  max=45.53ms p(90)=8.85ms  p(95)=11.99ms
     http_req_failed................: 0.00%   ✓ 0           ✗ 125334
     http_req_receiving.............: avg=85.51µs  min=14.7µs   med=24.1µs  max=34.94ms p(90)=40.7µs  p(95)=56.5µs 
     http_req_sending...............: avg=70.77µs  min=7µs      med=11.1µs  max=24.11ms p(90)=21.4µs  p(95)=28.1µs 
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s      max=0s      p(90)=0s      p(95)=0s     
     http_req_waiting...............: avg=3.99ms   min=321.41µs med=3.01ms  max=44.55ms p(90)=8.3ms   p(95)=11.55ms
     http_reqs......................: 125334  2088.587312/s
     iteration_duration.............: avg=4.77ms   min=587.62µs med=3.52ms  max=58.26ms p(90)=10.39ms p(95)=13.07ms
     iterations.....................: 125334  2088.587312/s
     vus............................: 9       min=9         max=10  
     vus_max........................: 10      min=10        max=10  

@dotansimha dotansimha merged commit eb6f53b into main Jun 15, 2021
@dotansimha dotansimha deleted the esm branch June 15, 2021 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants