search

jspm / generator

JSPM Import Map Generator
Apache License 2.0
167 stars 21 forks source link

Extend the existing import-map when using htmlGenerate #117

Closed JayaKrishnaNamburu closed 2 years ago

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

JayaKrishnaNamburu commented 2 years ago

Extending the existing this.traceMap.map with the map from htmlGenerate. So, the htmlGenerate can be used for simple concatenation purposes. Breaks a test right now, since it's inheriting the this.traceMap.map. I think we might need a clean-up action after generating map or some flag to allow this contactnation.

+ actual - expected ... Lines skipped

  '\n' +
    '<!doctype html>\n' +
...
    '  </script>\n' +
    '  <link rel="modulepreload" href="/react.js" />\n' +
+   '  <link rel="modulepreload" href="https://ga.jspm.io/npm:react@17.0.2/cjs/react.production.min.js" />\n' +
    '  <link rel="modulepreload" href="https://ga.jspm.io/npm:react@17.0.2/index.js" />\n' +
    '<script type="module">\n' +
    "  import 'react';\n" +
    '</script>\n'
    at file:///Users/jkrishna/Documents/jspm/generator/test/html/indent.test.js:13:8 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '\n' +
    '<!doctype html>\n' +
    '  <!-- Generated by @jspm/generator - https://github.com/jspm/generator -->\n' +
    '  <script async src="https://ga.jspm.io/npm:es-module-shims@1.5.1/dist/es-module-shims.js" crossorigin="anonymous"></script>\n' +
    '  <script type="importmap">\n' +
    '  {\n' +
    '    "imports": {\n' +
    '      "object-assign": "/react.js",\n' +
    '      "react": "https://ga.jspm.io/npm:react@17.0.2/index.js"\n' +
    '    }\n' +
    '  }\n' +
    '  </script>\n' +
    '  <link rel="modulepreload" href="/react.js" />\n' +
    '  <link rel="modulepreload" href="https://ga.jspm.io/npm:react@17.0.2/cjs/react.production.min.js" />\n' +
    '  <link rel="modulepreload" href="https://ga.jspm.io/npm:react@17.0.2/index.js" />\n' +
    '<script type="module">\n' +
    "  import 'react';\n" +
    '</script>\n',
  expected: '\n' +
    '<!doctype html>\n' +
    '  <!-- Generated by @jspm/generator - https://github.com/jspm/generator -->\n' +
    '  <script async src="https://ga.jspm.io/npm:es-module-shims@1.5.1/dist/es-module-shims.js" crossorigin="anonymous"></script>\n' +
    '  <script type="importmap">\n' +
    '  {\n' +
    '    "imports": {\n' +
    '      "object-assign": "/react.js",\n' +
    '      "react": "https://ga.jspm.io/npm:react@17.0.2/index.js"\n' +
    '    }\n' +
    '  }\n' +
    '  </script>\n' +
    '  <link rel="modulepreload" href="/react.js" />\n' +
    '  <link rel="modulepreload" href="https://ga.jspm.io/npm:react@17.0.2/index.js" />\n' +
    '<script type="module">\n' +
    "  import 'react';\n" +
    '</script>\n',
  operator: 'strictEqual'
}

We can see a additional react tracing has been added to react

+   '  <link rel="modulepreload" href="https://ga.jspm.io/npm:react@17.0.2/cjs/react.production.min.js" />\n' +
guybedford commented 2 years ago

Can you please provide a test case that shows what this is solving in the first place?

JayaKrishnaNamburu commented 2 years ago

@guybedford here is the test-case https://github.com/jspm/generator/pull/117/files#diff-30b40beb688c9a52f105d884f8aee14ac0515ca1b84b1c0980df6b5f3ea2750eR1

guybedford commented 2 years ago

Thanks for adding a test case... what exactly is the issue that the test case doesn't work on current generator? It seems like it should?

JayaKrishnaNamburu commented 2 years ago

It works, but starting to break other tests. Before trying to fix them, I would like to know if this can be a valid use case to solve.

guybedford commented 2 years ago

@JayaKrishnaNamburu I mean what is the issue that this PR solves? Looking at the test I can't tell why this test fails against the main branch.

guybedford commented 2 years ago

@JayaKrishnaNamburu specifically I'm thinking here - I know of at least two bugs that might cause generation issues with versions and existing maps which I could fix, so knowing the test case and why it's failing in the first place would help to know if this is one of those bugs or indeed a new feature request.

JayaKrishnaNamburu commented 2 years ago

It's the test from indent.test.js that's failing. I am not sure why. We are passing a simple html with module react used. But, it seems to generate two html tags for react. One with production and one with local. I don't know if i am able to explain correctly sorry 😅 

<link rel="modulepreload" href="https://ga.jspm.io/npm:react@17.0.2/cjs/react.production.min.js" />\n' +
'  <link rel="modulepreload" href="https://ga.jspm.io/npm:react@17.0.2/index.js" />\n' +

It's adding two module preloads instead of just one. Where the import-map is just

 {
    "imports": {
      "object-assign": "/react.js"
    }
  }
guybedford commented 2 years ago

It would really help for communication to always include a clear description and test when creating a PR to begin with.

I'm happy to help, I'm just very short on time. I'll take a look in the next couple of days.

guybedford commented 2 years ago

Just cloned and ran this which was necessary to get the context, and I finally get the concept that you want to retain the existing map.

Is this because you have a set import map of possible packages that must be retained for dynamic imports that are untraceable by the generator?

If so, do you need to use the map as the source of truth or would it be possible to just rely on generator.install to prepopulate these packages?

JayaKrishnaNamburu commented 2 years ago

I feel, replying on pre-defined import-map provided by the inputMap would be much reliable to eliminate the duplicate than the generator.install

guybedford commented 2 years ago

I see, thanks, yes I agree inputMap should be authoritatively embedded without pruning.

So the reason for the test failures then is that they share the generator instance, and thus this.traceMap.map has contents from the previous run.

Let's rather store inputMap in the constructor of the Generator on the instance, and extend from that directly rather than using this.traceMap.map as the inputMap source.

JayaKrishnaNamburu commented 2 years ago

So the reason for the test failures then is that they share the generator instance, and thus this.traceMap.map has contents from the previous run.

Yes, exactly what i was trying to say before 😅

guybedford commented 2 years ago

Yes, exactly what i was trying to say before

I see, but you did not mention inputMap or provide an initial description which did, which was why it took me so long to follow and required me to clone the repo and actually work through the code in detail to follow it.

guybedford commented 2 years ago

This should now be fully supported by https://github.com/jspm/generator/pull/134.