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

Make Ext import extensions consitent #2222

Open
sodic opened this issue Aug 2, 2024 · 11 comments
Open

Make Ext import extensions consitent #2222

sodic opened this issue Aug 2, 2024 · 11 comments
Labels
dx shouldfix We should do/fix this at some point

Comments

@sodic
Copy link
Contributor

sodic commented Aug 2, 2024

Summary

Issue spotted with Wasp 0.14.0.

Our allowed external import extensions are all over the place:

  • LSP, Snippets, and wasp start don't agree on what's allowed.
  • Different rules apply for different declarations (depending on where Wasp uses the user's code).

Let's fix this.

Details

There are three implicit categories of external imports (that I've discovered so far, there might be more). Each import falls into one or more of the categories:

  1. Ext imports used by the SDK (e.g., Queries).
  2. Ext imports used by the server (e.g., server setup function, Queries).
  3. Ext imports used by the web-app (e.g., pages, client setup function).

Notice how Queries fall into two categories. That's because they're copied into the SDK and also directly imported on the server (we do this to give users better error messages).

Since different imports end up in different build contexts, they are subject to different constraints (one context allows certain extensions, the other allows other extensions), making the behavior look random and confusing to users.

We've made some attempts at fixing this, and we have issues that talk about the problem:

But we never fully committed to properly deciding what we allow and where. The situation got worse after 0.12.0, which introduced an additional context with its own set of import constraints (the SDK).

Solution

Craig made some good points here: #1363 (comment). He talks about two distinct but related problems:

  • Handling extensions in the Wasp file (this issue).
  • Handling extensions in user files (another issue, to be created).

We must:

  1. Decide which extensions we want to support. We'll discuss that here, there's also a (discord discussion).
  2. Implement proper import mappings in SDK generator #2224
  3. Implement proper mappings in the server generator.
  4. Implement proper mappings in the web app generator.

By "mappings" I mean "mappings in Haskell and resolve configurations in the project."

Extra problem with the SDK build:

It's possible for the SDK build to pass, leaving extensions to cause problems in runtime. It happens with relative import references to files without extensions, and it also happened here: #2096 (comment).

I'm not yet sure 100% how this is possible. I'm guessing:

  • TypeScript can resolve the files during the "Build SDK" step and doesn't change the extension.
  • The runtime can't resolve the same fails with the same extensions and reports an error.

Ideally, we'd find a way to break the build if the import will be causing issues in runtime.

@sodic sodic added dx shouldfix We should do/fix this at some point labels Aug 2, 2024
@sodic
Copy link
Contributor Author

sodic commented Aug 2, 2024

Let's discuss which extensions we want to allow in our Wasp file.

The reasonable options I'm aware of are:

  1. No extensions ( @src/queries regardless of the file's true extension)
  2. The file's true extension (@src/queries.ts for queries.ts, @src/queries.js for queries.js, etc).
  3. The file's "compiled extension" - This is relevant only for TS stuff (@src/queries.js for queries.ts, and @src/queries.jsx for queries.tsx).
  4. The file's "transpiled extension" - This is relevant only for "X" stuff (@src/queries.js for queries.jsx, @src/queries.ts for queries.tsx)
  5. Some combination of the above.

User feeback

Points raised by our users in Discord (thread):

  • Either 1 (because it's simple) or 2 (because it's explicit), but I prefer option 1
  • what about svgs for example? non-code files would be imported with file extensions though?
  • I would prefer option 2, although 1 doesn't sound that bad either, but since non-code could potentially be imported, it could mean a lot to have the extension explicitly stated

So we should also consider how we want to handle Ext imports of non-js files (and if we'll ever have any).

My thoughts

I'm leaning towards either option 1 or option 2, not both of them (our users seem to agree). I'll analyze this dilemma from option 1's point of view.

PROS of alowing only @src/queries (no extension)
  • Users won't be tempted to have the same base file name with different extensions (which we don't support).
  • Easier to swtich from ts to js (for users).
  • Fewer rules for LSP and mappings.
  • Simpler to implement (requires a subset of functionalities that we'd need to implement to support more extensions)
CONS of alowing only @src/queries (no extension)
  • Reduced flexibility for our users, less robust. Users will run into ext import errors more frequently.
  • Possibly confusing: "Why must I change the file name from something.js to something when something.js is its real name?
  • Might not play nicely with the TS SDK (especially if we name the appropriate field something like fileName and it doesn't contain the actual file name). We could name it module`, but that's possibly too technical.
  • What happens when we introduce non-code Ext imports?

Mapping procedure

In any case, these are all the possible mappings (ext import -> SDK import string):

  • @src/queries.jsx -> wasp/ext-src/queries.jsx
  • @src/queries.tsx -> wasp/ext-src/queries.jsx
  • @src/queries.js -> wasp/ext-src/queries.js
  • @src/queries.ts -> wasp/ext-src/queries.js
  • @src/queries -> Apply the mapping procedure to the real file name

@OlegGulevskyy
Copy link

OlegGulevskyy commented Aug 2, 2024

Reduced flexibility for our users, less robust. Users will run into ext import errors more frequently.

What kind of errors do you think of?

Possibly confusing: "Why must I change the file name from something.js to something when something.js is its real name?

I might not be objective, but this was never the point of confusion for me personally. Actually au contraire, I was confused when at some point working with node TS lib I needed to use .js extensions, whereas my files were in .ts. I just accepted it and made my peace with it, but the fact is - this is something you don't do when building react projects (bundlers handle all of this for you).

Might not play nicely with the TS SDK (especially if we name the appropriate field something like fileName and it doesn't contain the actual file name). We could name it module`, but that's possibly too technical.

I don't understand this at all 👀
Maybe another example / use case can make it through my thick skull.

What happens when we introduce non-code Ext imports?

The same thing as what most modern bundlers do - handle it "internally" and bundle it. For example, in vite react app (or any other frameworks for that matter), you import your JS / JSX files with a simple file name (without extension) and assets such as PNG, Svgs etc - with extension. I think this is a "expected" behaviour by majority (..I dare speaking for majority...)

@sodic
Copy link
Contributor Author

sodic commented Aug 6, 2024

What kind of errors do you think of?

I meant: If we only support extensionless imports, users will more often be correcting how they import things (than they would if we just supported everything).

I might not be objective, but this was never the point of confusion for me personally. Actually au contraire, I was confused when at some point working with node TS lib I needed to use .js extensions, whereas my files were in .ts

We had a misunderstanding here.
I didn't mean they'd be wondering "Why do I have import TS file with JS extension."
I meant they'd be wondering "Why do I have to import a JS/TS file without an extension when it does have it in reality.

I don't understand this at all 👀

Haha, no worries, the TS SDK is still in the works so it's normal you don't know the details. The basic idea is, once we implement the TS SDK, specifying files without extensions might be weirder than it is now. More specifically...

This

app.action('createTask', {
  // Import without extension
  fn: { name: 'createTask', file: 'actions' },
  entities: [Task],
  auth: true,
})

Might look stranger than this:

app.action('createTask', {
  // Import without extension
  fn: { name: 'createTask', file: 'actions.js' },
  entities: [Task],
  auth: true,
})

Since the field's name is file, one would probably expect to be allowed to write the file's true name.

For example, in vite react app (or any other frameworks for that matter), you import your JS / JSX files with a simple file name (without extension) and assets such as PNG, Svgs etc - with extension.

Makes sense. Another thing to maybe consider is what happens when we introduce multiple languages. Extensions may com in handy then (but discussing this is probably looking too far into the future).

@sodic
Copy link
Contributor Author

sodic commented Aug 6, 2024

@infomiho suggested (assuming we're sticking with the object ext import syntax) we do something like this:

app.action('createTask', {
  // Mind the key name changes
  fn: { import: 'createTask', from: 'actions' },
  entities: [Task],
  auth: true,
})

But ideally we'd come up with a plugin that imports stuff normally (this will help us with the LSP as well).

@OlegGulevskyy
Copy link

@infomiho suggested (assuming we're sticking with the object ext import syntax) we do something like this:

app.action('createTask', {
  // Mind the key name changes
  fn: { import: 'createTask', from: 'actions' },
  entities: [Task],
  auth: true,
})

But ideally we'd come up with a plugin that imports stuff normally (this will help us with the LSP as well).

crazy thought probably, but what about instead of

fn: { import: 'createTask', from: 'actions' },

you just import the function and pass it in, like

import { createTask } from "@/actions";
app.action('createTask', {
  // Mind the key name changes
  fn: createTask,
  entities: [Task],
  auth: true,
})

this removes posslbe ambiguity of how to call files + it is a clear and predictable by devs way.
but I don't know how much this complicates things, but I'd vote for this kind of functionality at any time of the day I think

@sodic
Copy link
Contributor Author

sodic commented Aug 8, 2024

I'd vote for this kind of functionality at any time of the day I think

Yeah, I think we all agree there 😄

but I don't know how much this complicates things

This is precisely the problem - neither do I (yet), but this approach is what I meant when I said:

But ideally we'd come up with a plugin that imports stuff normally (this will help us with the LSP as well).

In any case, I'll be looking into it in the following weeks. Let's hope it's simple enough!

@Martinsos
Copy link
Member

{ import: '...', from: '...' } is perfectly fine for now I think, and has no issues with extensions, as you said! We can try to add more fancier stuff on top later, this will need to be the founbdation in any case.

What did Discord vote say, from what I got most people are for "no extension" option?

@sodic
Copy link
Contributor Author

sodic commented Aug 19, 2024

{ import: '...', from: '...' } is perfectly fine for now I think, and has no issues with extensions, as you said!

It has issues with differentiating between named and default imports, but I forgot to mention it. Any ideas there?

@Martinsos
Copy link
Member

{ import: '...', from: '...' } is perfectly fine for now I think, and has no issues with extensions, as you said!

It has issues with differentiating between named and default imports, but I forgot to mention it. Any ideas there?

We don't allow default imports though, right? So in that way it is not an issue. If we wanted to allow for it, we could I guess add defaultImport hm.

@infomiho
Copy link
Contributor

infomiho commented Sep 2, 2024

@Martinsos we allow default imports in the Wasp file (just tested this now)

@Martinsos
Copy link
Member

@Martinsos we allow default imports in the Wasp file (just tested this now)

Ooooh you are right, I forgot. We don't allow multiple imports per one statement, but we do allow a default import, or one named import, no more no less.

Ok then we will have to figure this out @sodic , sorry for wrong information!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx shouldfix We should do/fix this at some point
Projects
None yet
Development

No branches or pull requests

4 participants