Skip to content

feat(config): ✨ Add excludeConvertNames in atlas-resource.json#40

Closed
izcream wants to merge 3 commits intoabstractFlo:version-3-1-refactorfrom
izcream:version-3-1-refactor
Closed

feat(config): ✨ Add excludeConvertNames in atlas-resource.json#40
izcream wants to merge 3 commits intoabstractFlo:version-3-1-refactorfrom
izcream:version-3-1-refactor

Conversation

@izcream
Copy link
Copy Markdown

@izcream izcream commented Nov 16, 2021

Add config to make user can handle what npm_module don't need to call convertNamedImports function

I try to import axios, jsonwebtoken into atlas and compile I will got warning message on image below

warning-message_import_default

then I have look up builded/bundle code module axios, jsonwebtoken was write import 'moduleName'; it's should be import moduleName from 'moduleName';

ps. not sure why rollup convert module to import 'moduleName' because function convertNamedImports was convert to right import syntax

Atlas have function to convert to default name import from source code. and its convert some legacy npm-module wrong.
this config will able to make user decide to convert name import or not
@Timo972
Copy link
Copy Markdown
Contributor

Timo972 commented Nov 16, 2021

Afaik nodeJS 14+ solves cjs imports in ejs by itself, maybe it would be better to completely remove convertNamedImports.
Correct me if i'm wrong.

@izcream
Copy link
Copy Markdown
Author

izcream commented Nov 16, 2021

on my node js v14.18.1 and rollup ^2.60.0 axios was compile and work fine. but in atlas if I pass axios to convertNamedImports function it's do not😂 btw if remove this function my issues about import will gone true😁

@Timo972
Copy link
Copy Markdown
Contributor

Timo972 commented Nov 16, 2021

thats what i'm talking about, i think node 14+ does not need convertNamedImport anymore because they changed the import behaviour

@abstractFlo
Copy link
Copy Markdown
Owner

But we need this on clientside? Can anyone check this? Clientside does not run a node environment. If they can handle as well, we can remove convertNamedImportes, otherwise not ;)

@izcream
Copy link
Copy Markdown
Author

izcream commented Nov 17, 2021

When I try to import axios on client side it was fail to bundle its. Tonight will try without convertNameImport and update result again😅

@Timo972
Copy link
Copy Markdown
Contributor

Timo972 commented Nov 17, 2021

Axios will never work on clientside only alt.HttpClient

@izcream
Copy link
Copy Markdown
Author

izcream commented Nov 17, 2021

updated import axios,jsonwebtoken in client side with and without convertNameImports function both build failed with error message in pic below

image

@Timo972
Copy link
Copy Markdown
Contributor

Timo972 commented Nov 17, 2021

updated import axios,jsonwebtoken in client side with and without convertNameImports function both build failed with error message in pic below

image

you are trying to import a nodejs module on clientside which is not possible since the client script runtime is using v8.

@izcream
Copy link
Copy Markdown
Author

izcream commented Nov 17, 2021

But we need this on clientside? Can anyone check this? Clientside does not run a node environment. If they can handle as well, we can remove convertNamedImportes, otherwise not ;)

yes I know it cant. Just make a test result for abstracFlo comment above 😂

@izcream
Copy link
Copy Markdown
Author

izcream commented Nov 18, 2021

Update info from Timo and abstractFlo Look like wee don't need convertNamedImports function anymore because function was use on Serverside Config only(not found this function has use by another class on project) so. I decided to remove ConvertNamedImports function to this pull request. can someone recheck this pr?

Thanks

@abstractFlo
Copy link
Copy Markdown
Owner

I don't remove the plugin currently, i will check it by myself is it usefull to remove this. I can't accept this PR, sorry

@izcream
Copy link
Copy Markdown
Author

izcream commented Nov 18, 2021

Ok so can you add ability to make user choice to convertNameImport or not like my fist commit?

With this options I can solve import wrong for old npm modules😅

@abstractFlo
Copy link
Copy Markdown
Owner

Currently i have no time doing something related to altas :/ sorry

But the time is coming, i am on final stage in my current private project and if this is finished, i will push all my improvements to v3.1

@izcream
Copy link
Copy Markdown
Author

izcream commented Nov 18, 2021

Thanks for supporting for make v3 release. If you can fix my issues that will easy to me to handle atlas module(no need to fork and fix this myself)

edited:

The main problem is not whole convertNamedImports function another part on this function still need to have

only this sub function inside convertNamedImports make the issues happen

root.find(j.ImportDeclaration).map((path) => {
    let moduleName = path.value.source.value;
    moduleMap[moduleName] = [];

    if (moduleName.startsWith('.') || !modulesForConvert.includes(moduleName) || !path.node.specifiers.length)
      return;

    path.node.specifiers.forEach((specifier) => {
      if (specifier.type !== 'ImportSpecifier') return;

      moduleMap[moduleName].push(specifier.local.name);
    });

    j(path).replaceWith(
        `import ${!config.useStarImport ? '' : '* as'} ${pascalCase(moduleName)} from '${moduleName}';`
    );
  });

Step to reproduce this issues


  1. import axios or jsonwebtoken module on server-side code
  2. compile script (will receive rollup warning message about "default" import)
  3. run altv-server and you will receive undefined axios, jsonwebtoken error

Hope you and your team can fix this issues soon, Have a nice day :)

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