PoC ESM support #2563
Closed
PoC ESM support #2563
+3,194
−1,047
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
codebien
reviewed
Aug 4, 2022
| } | ||
|
|
||
| // This goja.ModuleRecord wrapper for go/js module which does not conform to modules.Module interface | ||
| type wrappedBasicGoModule struct { |
There was a problem hiding this comment.
Suggested change
| type wrappedBasicGoModule struct { | |
| type uncompliantModule struct { |
Just as a suggestion for searching a better name.
| } | ||
|
|
||
| // This goja.ModuleRecord wrapper for go/js module which conforms to modules.Module interface | ||
| type wrappedGoModule struct { |
There was a problem hiding this comment.
Suggested change
| type wrappedGoModule struct { | |
| type esModule struct { |
| "go.k6.io/k6/js/modules" | ||
| ) | ||
|
|
||
| func wrapGoModule(mod interface{}) goja.ModuleRecord { |
There was a problem hiding this comment.
Suggested change
| func wrapGoModule(mod interface{}) goja.ModuleRecord { | |
| func resolveModuleRecord(mod interface{}) goja.ModuleRecord { |
| } | ||
|
|
||
| // TODO use the first argument | ||
| func (b *Bundle) resolveModule(ref interface{}, specifier string) (goja.ModuleRecord, error) { |
There was a problem hiding this comment.
It's a very long method and a bit difficult to follow/review, can we split it a bit?
This was referenced Sep 7, 2022
mstoykov
added a commit
that referenced
this pull request
Jan 27, 2023
This refactor tries to simplify the implementation of `require` and connected code by splitting it heavily into different parts. This changes are similar to what will be needed for native ESM support as shown in #2563 , but without any of the native parts and without doing anything that isn't currently needed. This includes still keeping the wrong relativity of `require` as explained in #2674. It also tries to simplify connected code, but due to this being very sensitive code and the changes already being quite big, this is done only to an extend. The lack of new tests is mostly due to there not being really any new code and the tests that were created along this changes already being merged months ago with #2782. Future changes will try to address the above as well as potentially moving the whole module types and logic in separate package to be reused in tests.
mstoykov
added a commit
that referenced
this pull request
Jan 27, 2023
This refactor tries to simplify the implementation of `require` and connected code by splitting it heavily into different parts. These changes are similar to what will be needed for native ESM support as shown in #2563 , but without any of the native parts and without doing anything that isn't currently needed. This will hopefully make ESM PR much smaller and less intrusive. This includes still keeping the wrong relativity of `require` as explained in #2674. It also tries to simplify connected code, but due to this being very sensitive code and the changes already being quite big, this is done only to an extent. The lack of new tests is mostly due to there not being really any new code and the tests that were created along these changes already being merged months ago with #2782. Future changes will try to address the above as well as potentially moving the whole module types and logic in separate package to be reused in tests.
mstoykov
added a commit
that referenced
this pull request
Jan 27, 2023
This refactor tries to simplify the implementation of `require` and connected code by splitting it heavily into different parts. These changes are similar to what will be needed for native ESM support as shown in #2563 , but without any of the native parts and without doing anything that isn't currently needed. This will hopefully make ESM PR much smaller and less intrusive. This includes still keeping the wrong relativity of `require` as explained in #2674. It also tries to simplify connected code, but due to this being very sensitive code and the changes already being quite big, this is done only to an extent. The lack of new tests is mostly due to there not being really any new code and the tests that were created along these changes already being merged months ago with #2782. Future changes will try to address the above as well as potentially moving the whole module types and logic in separate package to be reused in tests.
mstoykov
added a commit
that referenced
this pull request
Feb 2, 2023
This refactor tries to simplify the implementation of `require` and connected code by splitting it heavily into different parts. These changes are similar to what will be needed for native ESM support as shown in #2563 , but without any of the native parts and without doing anything that isn't currently needed. This will hopefully make ESM PR much smaller and less intrusive. This includes still keeping the wrong relativity of `require` as explained in #2674. It also tries to simplify connected code, but due to this being very sensitive code and the changes already being quite big, this is done only to an extent. The lack of new tests is mostly due to there not being really any new code and the tests that were created along these changes already being merged months ago with #2782. Future changes will try to address the above as well as potentially moving the whole module types and logic in separate package to be reused in tests.
mstoykov
added a commit
that referenced
this pull request
Feb 2, 2023
This refactor tries to simplify the implementation of `require` and connected code by splitting it heavily into different parts. These changes are similar to what will be needed for native ESM support as shown in #2563 , but without any of the native parts and without doing anything that isn't currently needed. This will hopefully make ESM PR much smaller and less intrusive. This includes still keeping the wrong relativity of `require` as explained in #2674. It also tries to simplify connected code, but due to this being very sensitive code and the changes already being quite big, this is done only to an extent. The lack of new tests is mostly due to there not being really any new code and the tests that were created along these changes already being merged months ago with #2782. Future changes will try to address the above as well as potentially moving the whole module types and logic in separate package to be reused in tests.
mstoykov
added a commit
that referenced
this pull request
Feb 8, 2023
This refactor tries to simplify the implementation of `require` and connected code by splitting it heavily into different parts. These changes are similar to what will be needed for native ESM support as shown in #2563 , but without any of the native parts and without doing anything that isn't currently needed. This will hopefully make ESM PR much smaller and less intrusive. This includes still keeping the wrong relativity of `require` as explained in #2674. It also tries to simplify connected code, but due to this being very sensitive code and the changes already being quite big, this is done only to an extent. The lack of new tests is mostly due to there not being really any new code and the tests that were created along these changes already being merged months ago with #2782. Future changes will try to address the above as well as potentially moving the whole module types and logic in separate package to be reused in tests.
mstoykov
added a commit
that referenced
this pull request
Mar 7, 2023
This refactor tries to simplify the implementation of `require` and connected code by splitting it heavily into different parts. These changes are similar to what will be needed for native ESM support as shown in #2563 , but without any of the native parts and without doing anything that isn't currently needed. This will hopefully make ESM PR much smaller and less intrusive. This includes still keeping the wrong relativity of `require` as explained in #2674. It also tries to simplify connected code, but due to this being very sensitive code and the changes already being quite big, this is done only to an extent. The lack of new tests is mostly due to there not being really any new code and the tests that were created along these changes already being merged months ago with #2782. Future changes will try to address the above as well as potentially moving the whole module types and logic in separate package to be reused in tests.
|
Superseeded by #3456 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is not a final PR it is here mostly to discuss whether the changes presented here are satisfying our needs.
This PR will need to be merged after the goja PR (to be done) with the ESM support is merged.
TODO:
requireandopenas they need GetActiveScriptOrModule from the specification to be written correctly ;(.