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

feat: Deprecate "import assertions" with a warning #24743

Merged
merged 15 commits into from
Aug 19, 2024

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Jul 26, 2024

This commit deprecates "import assertions" proposal that has been
replaced with "import attributes".

Any time an import assertion is encountered a warning will be printed
to the terminal. This warning will be printed for both local and
remote files (ie. user code and dependencies).

Import assertions support will be removed in Deno 2.

@marvinhagemeister
Copy link
Contributor

Should this be a linting rule so that users can call deno lint --fix to update their code?

@bartlomieju
Copy link
Member Author

I'm not sure if that's feasible, I don't see anything in SWC's AST that would indicate that; see https://swc-ast.vercel.app/ and paste import data from "./data.json" assert { type: "json" };. The produced AST contains "with" property, but there's no indication of "assert" keyword. @dsherret thoughts on this one?

@bartlomieju
Copy link
Member Author

Also, @lucacasonato wants us to warn on usage of "assert" in the dependencies, which deno lint would not catch.

I'm currently fighting with V8 though, it appears that if V8 code cache is used then the warning is not printed - ie. only the first time you execute the warning is printed.

@bartlomieju
Copy link
Member Author

Did more digging and the problem with message appearing only once is indeed the V8 code cache. So to land this PR we need to do a few things:

  • ensure that V8 code cache is not saved (or purged) for modules that raised this warning
  • capture all messages from V8 and filter out all but the one for import assertion
  • forward information about module that raised the warning and make it drop V8 code cache for this module

@bartlomieju bartlomieju added this to the 1.46 milestone Jul 28, 2024
bartlomieju added a commit to denoland/deno_core that referenced this pull request Aug 1, 2024
@bartlomieju bartlomieju force-pushed the warn_on_import_assertions branch from 154e7cf to 59a518b Compare August 1, 2024 15:42
@bartlomieju bartlomieju changed the title [WIP] feat: Warn on usage of import assertions feat: Deprecate "import assertions" with a warning Aug 1, 2024
@bartlomieju bartlomieju marked this pull request as ready for review August 1, 2024 22:26
@bartlomieju
Copy link
Member Author

Blocker for landing: this still only prints warning once, after that if V8 cache is used the warning will not be printed again.

@bartlomieju
Copy link
Member Author

Blocked on denoland/deno_core#873.

cli/module_loader.rs Outdated Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

// NOTE(bartlomieju): this is temporary, for deprecated import assertions.
// Should be removed in Deno 2.
// Modules stored here should not be V8 code-cached.
prevent_v8_code_cache: Arc<Mutex<HashSet<String>>>,
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be an in Arc? Maybe use DashSet instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's a big deal? This code is gonna be gone in a few days anyway.

@bartlomieju bartlomieju merged commit b5051e2 into denoland:main Aug 19, 2024
17 checks passed
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.

3 participants