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

parser: add parse-only support for "import.meta" #6958

Closed
wants to merge 3 commits into from

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Oct 2, 2018

Summary:
Resolves #6913. As with new.target, we can parse the expression, but
we fail to typecheck it. Clients can use a suppression comment to ignore
uses of import.meta in an otherwise valid file.

The parse error message on inputs like import.notMeta was chosen to be
consistent with the message emitted by Babylon.

Test Plan:
Unit tests added, and all existing tests pass.

wchargin-branch: parse-import-meta

Summary:
Resolves facebook#6913. As with `new.target`, we can parse the expression, but
we fail to typecheck it. Clients can use a suppression comment to ignore
uses of `import.meta` in an otherwise valid file.

The parse error message on inputs like `import.notMeta` was chosen to be
consistent with the message emitted by Babylon.

Test Plan:
Unit tests added, and all existing tests pass.

wchargin-branch: parse-import-meta
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mroch is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@goodmind
Copy link
Contributor

@mroch is this still on your radar?

@goodmind goodmind added the Stalled Issues and PRs that are stalled. label Jul 27, 2019
@goodmind
Copy link
Contributor

/cc @mroch

@goodmind
Copy link
Contributor

@mroch

1 similar comment
@fredericgermain
Copy link

@mroch

@wchargin
Copy link
Contributor Author

I went ahead and merged in changes over the last year, fixed conflicts,
upgraded past a major OPAM and OCaml version boundary, installed bwrap
from source after finding all its dependencies, revised the code due to
some changes in Flow internals, and added a new test. @mroch, it’d be
great if this could be landed so that I don’t have to do this all again
next year! :-)

@wchargin wchargin force-pushed the wchargin-parse-import-meta branch from 2dbb33d to 400f03c Compare September 30, 2019 00:57
@wchargin wchargin force-pushed the wchargin-parse-import-meta branch from 400f03c to a9190bc Compare September 30, 2019 01:49
@mroch mroch added the ES2020 label Apr 16, 2020
@wchargin
Copy link
Contributor Author

wchargin commented Oct 2, 2020

Bernie Sanders meme: “I am once again asking for your code review”

Happy birthday! This pull request is two years old. As such, I’ve again
resolved the merge conflicts due to upstream changes, and gotten the
tests passing. @jbrown215, it’d be great if this could be landed so that
I don’t have to do this all again again next year! :-)

@fredericgermain
Copy link

You're so brave @wchargin

This issue is the reason I'm going to TypeScript.

I thought Flow was a better idea overall than TypeScript, but the latest versions and the governance of the project made me lose trust in it.

Happy birthday to the issue!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jbrown215 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jbrown215
Copy link
Contributor

From @mroch:

I don't think the locs are correct in the error cases, they consume tokens that aren't included in the range and don't point at the entire error (AFAICT).
IMO, should still use with_loc but also grab start_loc if it's needed for that error_at call.

The lint errors are ocamlformat errors. I can fix that on my side!

@FezVrasta
Copy link
Contributor

Any chance to get this PR merged?

@TrySound
Copy link
Contributor

Hi @jbrown215 @mroch! Any new on this feature?

@ancms2600
Copy link

Get it merged, you turkeys

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 0851b3e.

@isker
Copy link
Contributor

isker commented Sep 2, 2021

Bless.

@gkz
Copy link
Member

gkz commented Sep 2, 2021

@ancms2600

Get it merged, you turkeys

There you go
🦃

@FezVrasta
Copy link
Contributor

Is there any way to type import.meta.env?

@gkz
Copy link
Member

gkz commented Aug 23, 2022

What should that be? Do you have links to documentation?

@FezVrasta
Copy link
Contributor

FezVrasta commented Aug 23, 2022

It's used by Vite to access environment variables. It works identically to process.env

@gkz
Copy link
Member

gkz commented Aug 23, 2022

If I add a [string]: mixed indexer to the type, so you can access random other properties (but they're mixed so you have to refine them first to e.g. a string), would that be fine? We probably don't want to hardcode random specific things like vite env vars in Flow.

So you could do

const env = import.meta.env;
if (typeof env === 'string') {
  // do the stuff
}

@FezVrasta
Copy link
Contributor

I think it would be fine, anyways most of the times all you do is doing a string comparison so even if it's mixed it will work just fine.

Of course having a way to extend such types in user land would be optimal, as import.meta is thought to be user extensible.

facebook-github-bot pushed a commit that referenced this pull request Aug 27, 2022
…property lookup

Summary:
"The import.meta object exposes context-specific metadata to a JavaScript module."
This means it can contain arbitrary data. To allow our users to access this data in a safe way, we can add a `[string]: mixed` indexer.

Requested in GitHub: #6958 (comment)

Changelog: [feature] Allow access of arbitrary properties from `import.meta`. They are typed as `mixed`

Reviewed By: SamChou19815

Differential Revision: D39072638

fbshipit-source-id: 099dd4b17755210d8a893edf2c2c9a94ae38bd74
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flow parser should support import.meta