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

Support import.meta and import.meta.* #5319

Merged
merged 9 commits into from
May 30, 2020
Merged

Conversation

aurium
Copy link
Contributor

@aurium aurium commented May 10, 2020

Allows to access import.meta and only if its property is meta.

The new token IMPORT_META was created to allow richer formatting and code analysis.

$ ./bin/coffee --tokens -e 'import.meta.something'
[IMPORT_META import] [. .] [PROPERTY meta] [. .] [PROPERTY something] [TERMINATOR \n]

closes #5317

aurium added 2 commits May 8, 2020 22:56
It adds a new token `IMPORT_META` allowing richer formatting and code analysis.
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very clean. @helixbass?

@vendethiel
Copy link
Collaborator

Maybe not important, but this breaks indented property access like

a = import
 .meta

# or (forgive me...)
a = import.
  meta

@aurium
Copy link
Contributor Author

aurium commented May 11, 2020

@vendethiel, now it can. Thanks!

@GeoffreyBooth
Copy link
Collaborator

this breaks indented property access

The same is true of new.target, so if we allow it for import.meta we should allow it there too.

I'm conflicted, as these aren't actually properties of an object the way that obj.prop is. import isn't an object, nor is it a function (even though it can appear like one e.g. import()). It's just a weird piece of syntax. Though I suppose the 'property access' is a part of grammar, and import.meta/new.target uses that grammar, so maybe it should follow that pattern?

Copy link
Collaborator

@helixbass helixbass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aurium this is mostly looking good, see comments

And please add AST tests to test/abstract_syntax_tree.coffee and test/abstract_syntax_tree_location.data.coffee (you can add tests under the existing MetaProperty node tests if you want - again you should be able to use the new.target tests as a reference, though for import.meta you don't need to nest inside a function body if you don't want)

I'm happy to re-review once comments are addressed

src/grammar.coffee Show resolved Hide resolved
src/lexer.coffee Outdated Show resolved Hide resolved
test/modules.coffee Outdated Show resolved Hide resolved
@vendethiel
Copy link
Collaborator

vendethiel commented May 11, 2020

@aurium I meant the approach in general is different from how this is usually done. Might not matter in this specific case (because it's not gonna be an issue in practice), but you're always gonna break some code if you "revert" to regex scanning.

b.
  # Here's a nice comment about the meta property
 c

See @helixbass 's comment on "retroactively updating a token"

@GeoffreyBooth
Copy link
Collaborator

@helixbass I added your excellent explanation as a comment in afab371 on this branch.

@aurium I merged the latest master into this branch. This adds a new and improved CI that includes testing the browser compiler, and uses GitHub Actions.

Have all of the review notes been addressed?

@aurium
Copy link
Contributor Author

aurium commented May 28, 2020

I believe... yes.

@GeoffreyBooth
Copy link
Collaborator

I believe... yes.

Great! @helixbass and @vendethiel, do you have any more notes?

@vendethiel
Copy link
Collaborator

LGTM

1 similar comment
@helixbass
Copy link
Collaborator

LGTM

@GeoffreyBooth GeoffreyBooth merged commit 2fd9ee4 into jashkenas:master May 30, 2020
@gcxfd
Copy link

gcxfd commented Apr 24, 2021

when where release this version ?

@GeoffreyBooth GeoffreyBooth mentioned this pull request Sep 19, 2021
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.

Grammar, Nodes: Support import.meta and import.meta.*
5 participants