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

ESM discussion: open and require are relative to? #2674

Closed
mstoykov opened this issue Sep 1, 2022 · 1 comment
Closed

ESM discussion: open and require are relative to? #2674

mstoykov opened this issue Sep 1, 2022 · 1 comment
Assignees
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 question

Comments

@mstoykov
Copy link
Collaborator

mstoykov commented Sep 1, 2022

Background:

Currently, require and open are both relative to the current root module/script that is being executed. This is best explain with an example:

If you have.

main.js that imports 'folder/B.js'

// this is main.js
import { a } from "./folder/B.js";
var somefile = a(); // this will open `./data.json`
export default() {}
export function a () {
   return open("./data.json");
}
a() // this will open `./folder/data.json`

The same goes for require.

Problem/question:

import() (the dynamic import) is defined as being relative to the file it's being written in as in the above case if you replace open with import it will in both instances open ./folder/data.json.

The same is technically true for how require is defined :( , from https://wiki.commonjs.org/wiki/Website:Index/specs/modules/1.0

Relative identifiers are resolved relative to the identifier of the module in which "require" is written and called.

Arguably both require and open will be almost always be used in the top context of the module/script, so it wouldn't matter - they will just always be relative to the file they are written in.

So the questions are two:

  1. should require be fixed to be relative to the module it is in ... basically the same as import() and as it's described?
  2. should open be the same just to not be confusing?

From implementation perspective making it all the same will be way easier in ESM as getting the root module/script that is currently running isn't a thing goja currently supports and likely will require some hacks.

@mstoykov mstoykov added question evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels Sep 1, 2022
mstoykov added a commit that referenced this issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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.
@mstoykov
Copy link
Collaborator Author

Closing in favor of #3857

@mstoykov mstoykov closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 question
Projects
None yet
Development

No branches or pull requests

1 participant